xiaoyuyao commented on a change in pull request #1874:
URL: https://github.com/apache/ozone/pull/1874#discussion_r568792493
##########
File path:
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/authority/CertificateServer.java
##########
@@ -101,26 +101,22 @@ X509Certificate getCertificate(String certSerialId)
/**
* Revokes a Certificate issued by this CertificateServer.
*
- * @param certificate - Certificate to revoke
- * @param approver - Approval process to follow.
+ * @param certificates - List of Certificates to revoke.
+ * @param reason - Reason for revocation.
+ * @param securityConfig - Security Configuration.
* @return Future that tells us what happened.
- * @throws SCMSecurityException - on Error.
- */
- Future<Boolean> revokeCertificate(X509Certificate certificate,
- ApprovalType approver) throws SCMSecurityException;
-
- /**
- * TODO : CRL, OCSP etc. Later. This is the start of a CertificateServer
- * framework.
*/
+ Future<Boolean> revokeCertificates(List<X509Certificate> certificates,
+ int reason,
Review comment:
Can the reason be a Enum?
##########
File path:
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/authority/CertificateServer.java
##########
@@ -101,26 +101,22 @@ X509Certificate getCertificate(String certSerialId)
/**
* Revokes a Certificate issued by this CertificateServer.
*
- * @param certificate - Certificate to revoke
- * @param approver - Approval process to follow.
+ * @param certificates - List of Certificates to revoke.
+ * @param reason - Reason for revocation.
+ * @param securityConfig - Security Configuration.
* @return Future that tells us what happened.
- * @throws SCMSecurityException - on Error.
- */
- Future<Boolean> revokeCertificate(X509Certificate certificate,
- ApprovalType approver) throws SCMSecurityException;
-
- /**
- * TODO : CRL, OCSP etc. Later. This is the start of a CertificateServer
- * framework.
Review comment:
NIT: javadoc?
##########
File path:
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/authority/DefaultCAServer.java
##########
@@ -272,20 +272,22 @@ private X509CertificateHolder
signAndStoreCertificate(LocalDate beginDate,
}
@Override
- public Future<Boolean> revokeCertificate(X509Certificate certificate,
- CertificateApprover.ApprovalType approverType)
- throws SCMSecurityException {
+ public Future<Boolean> revokeCertificates(List<X509Certificate> certificates,
+ int reason,
+ SecurityConfig securityConfig) {
CompletableFuture<Boolean> revoked = new CompletableFuture<>();
- if (certificate == null) {
+ if (certificates == null || certificates.isEmpty()) {
Review comment:
NIT: we can use Collection#isEmptyOrNull here.
##########
File path:
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/authority/CertificateStore.java
##########
@@ -47,12 +50,21 @@ void storeValidCertificate(BigInteger serialID,
X509Certificate certificate) throws IOException;
/**
- * Moves a certificate in a transactional manner from valid certificate to
+ * Adds the certificates to be revoked to a new CRL and moves all the
+ * certificates in a transactional manner from valid certificate to
* revoked certificate state.
- * @param serialID - Serial ID of the certificate.
+ * @param certificates - List of X509 Certificates to be revoked.
+ * @param caCertificateHolder - X509 Certificate Holder of the CA.
+ * @param reason - CRLReason for revocation.
+ * @param securityConfig - Security Configuration.
+ * @param keyPair - Public and Private key of the CA.
* @throws IOException
*/
- void revokeCertificate(BigInteger serialID) throws IOException;
+ void revokeCertificates(List<X509Certificate> certificates,
+ X509CertificateHolder caCertificateHolder,
+ int reason, SecurityConfig securityConfig,
+ KeyPair keyPair)
Review comment:
can we abstract the CRL handling into a separate class (like Approver
for CSR) without bringing CA keypair into the certificate store interface and
class?
##########
File path:
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/crl/CRLInfo.java
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.security.x509.crl;
+
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.hadoop.hdds.security.exception.SCMSecurityException;
+import org.apache.hadoop.hdds.security.x509.certificate.utils.CRLCodec;
+import org.jetbrains.annotations.NotNull;
+
+import java.io.IOException;
+import java.security.cert.CRLException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509CRL;
+import java.util.Comparator;
+import java.util.Objects;
+
+/**
+ * Class that wraps Certificate Revocation List Info.
+ */
+public class CRLInfo implements Comparator<CRLInfo>,
+ Comparable<CRLInfo> {
+
+ private X509CRL x509CRL;
+ private long creationTimestamp;
+
+ private CRLInfo(X509CRL x509CRL, long creationTimestamp) {
+ this.x509CRL = x509CRL;
+ this.creationTimestamp = creationTimestamp;
+ }
+
+ /**
+ * Constructor for CRLInfo. Needed for serialization findbugs.
+ */
+ public CRLInfo() {
+ }
+
+ public static CRLInfo fromProtobuf(HddsProtos.CRLInfoProto info)
+ throws IOException, CRLException, CertificateException {
+ CRLInfo.Builder builder = new CRLInfo.Builder();
+ return builder
+ .setX509CRL(CRLCodec.getX509CRL(info.getX509CRL()))
+ .setCreationTimestamp(info.getCreationTimestamp())
+ .build();
+ }
+
+ public HddsProtos.CRLInfoProto getProtobuf() throws SCMSecurityException {
+ HddsProtos.CRLInfoProto.Builder builder =
+ HddsProtos.CRLInfoProto.newBuilder();
+
+ return builder.setX509CRL(CRLCodec.getPEMEncodedString(getX509CRL()))
+ .setCreationTimestamp(getCreationTimestamp())
+ .build();
+ }
+
+ public X509CRL getX509CRL() {
+ return x509CRL;
+ }
+
+ public long getCreationTimestamp() {
+ return creationTimestamp;
+ }
+
+ /**
+ * Compares this object with the specified object for order. Returns a
+ * negative integer, zero, or a positive integer as this object is less
+ * than, equal to, or greater than the specified object.
+ *
+ * @param o the object to be compared.
+ * @return a negative integer, zero, or a positive integer as this object
+ * is less than, equal to, or greater than the specified object.
+ * @throws NullPointerException if the specified object is null
+ * @throws ClassCastException if the specified object's type prevents it
+ * from being compared to this object.
+ */
+ @Override
+ public int compareTo(@NotNull CRLInfo o) {
+ return this.compare(this, o);
+ }
+
+ /**
+ * Compares its two arguments for order. Returns a negative integer,
+ * zero, or a positive integer as the first argument is less than, equal
+ * to, or greater than the second.<p>
+ * <p>
+ *
+ * @param o1 the first object to be compared.
+ * @param o2 the second object to be compared.
+ * @return a negative integer, zero, or a positive integer as the
+ * first argument is less than, equal to, or greater than the
+ * second.
+ * @throws NullPointerException if an argument is null and this
+ * comparator does not permit null arguments
+ * @throws ClassCastException if the arguments' types prevent them from
+ * being compared by this comparator.
+ */
+ @Override
+ public int compare(CRLInfo o1, CRLInfo o2) {
+ return 0;
Review comment:
This does not seem to be implemented yet?
##########
File path:
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/crl/CRLInfo.java
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.security.x509.crl;
+
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.hadoop.hdds.security.exception.SCMSecurityException;
+import org.apache.hadoop.hdds.security.x509.certificate.utils.CRLCodec;
+import org.jetbrains.annotations.NotNull;
+
+import java.io.IOException;
+import java.security.cert.CRLException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509CRL;
+import java.util.Comparator;
+import java.util.Objects;
+
+/**
+ * Class that wraps Certificate Revocation List Info.
+ */
+public class CRLInfo implements Comparator<CRLInfo>,
+ Comparable<CRLInfo> {
+
+ private X509CRL x509CRL;
+ private long creationTimestamp;
+
+ private CRLInfo(X509CRL x509CRL, long creationTimestamp) {
+ this.x509CRL = x509CRL;
+ this.creationTimestamp = creationTimestamp;
+ }
+
+ /**
+ * Constructor for CRLInfo. Needed for serialization findbugs.
+ */
+ public CRLInfo() {
+ }
+
+ public static CRLInfo fromProtobuf(HddsProtos.CRLInfoProto info)
+ throws IOException, CRLException, CertificateException {
+ CRLInfo.Builder builder = new CRLInfo.Builder();
+ return builder
+ .setX509CRL(CRLCodec.getX509CRL(info.getX509CRL()))
+ .setCreationTimestamp(info.getCreationTimestamp())
+ .build();
+ }
+
+ public HddsProtos.CRLInfoProto getProtobuf() throws SCMSecurityException {
+ HddsProtos.CRLInfoProto.Builder builder =
+ HddsProtos.CRLInfoProto.newBuilder();
+
+ return builder.setX509CRL(CRLCodec.getPEMEncodedString(getX509CRL()))
+ .setCreationTimestamp(getCreationTimestamp())
+ .build();
+ }
+
+ public X509CRL getX509CRL() {
+ return x509CRL;
+ }
+
+ public long getCreationTimestamp() {
+ return creationTimestamp;
+ }
+
+ /**
+ * Compares this object with the specified object for order. Returns a
+ * negative integer, zero, or a positive integer as this object is less
+ * than, equal to, or greater than the specified object.
+ *
+ * @param o the object to be compared.
+ * @return a negative integer, zero, or a positive integer as this object
+ * is less than, equal to, or greater than the specified object.
+ * @throws NullPointerException if the specified object is null
+ * @throws ClassCastException if the specified object's type prevents it
+ * from being compared to this object.
+ */
+ @Override
+ public int compareTo(@NotNull CRLInfo o) {
+ return this.compare(this, o);
+ }
+
+ /**
+ * Compares its two arguments for order. Returns a negative integer,
+ * zero, or a positive integer as the first argument is less than, equal
+ * to, or greater than the second.<p>
+ * <p>
+ *
+ * @param o1 the first object to be compared.
+ * @param o2 the second object to be compared.
+ * @return a negative integer, zero, or a positive integer as the
+ * first argument is less than, equal to, or greater than the
+ * second.
+ * @throws NullPointerException if an argument is null and this
+ * comparator does not permit null arguments
+ * @throws ClassCastException if the arguments' types prevent them from
+ * being compared by this comparator.
+ */
+ @Override
+ public int compare(CRLInfo o1, CRLInfo o2) {
+ return 0;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+
+ CRLInfo that = (CRLInfo) o;
+
+ return this.getX509CRL().equals(that.x509CRL) &&
+ this.creationTimestamp == that.creationTimestamp;
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(x509CRL);
+ }
+
Review comment:
Can we add a toString override for debugging output, etc?
##########
File path:
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/x509/certificate/authority/TestDefaultCAServer.java
##########
@@ -213,6 +224,55 @@ public void testRequestCertificateWithInvalidSubject()
throws IOException,
assertNotNull(holder.get());
}
+ @Test
+ public void testRevokeCertificates() throws Exception {
+ String scmId = RandomStringUtils.randomAlphabetic(4);
+ String clusterId = RandomStringUtils.randomAlphabetic(4);
+
+ CertificateServer testCA = new DefaultCAServer("testCA",
+ clusterId, scmId, caStore);
+ testCA.init(new SecurityConfig(conf),
+ CertificateServer.CAType.SELF_SIGNED_CA);
+
+ KeyPair keyPair =
+ new HDDSKeyGenerator(conf).generateKey();
+ PKCS10CertificationRequest csr = new CertificateSignRequest.Builder()
+ .addDnsName("hadoop.apache.org")
+ .addIpAddress("8.8.8.8")
+ .setCA(false)
+ .setSubject("testCA")
+ .setConfiguration(conf)
+ .setKey(keyPair)
+ .build();
+
+ // Let us convert this to a string to mimic the common use case.
+ String csrString = CertificateSignRequest.getEncodedString(csr);
+
+ Future<X509CertificateHolder> holder = testCA.requestCertificate(csrString,
+ CertificateApprover.ApprovalType.TESTING_AUTOMATIC);
+
+ X509Certificate certificate =
+ new JcaX509CertificateConverter().getCertificate(holder.get());
+ List<X509Certificate> certs = new ArrayList<>();
+ certs.add(certificate);
+ Future<Boolean> revoked = testCA.revokeCertificates(certs,
+ CRLReason.keyCompromise, new SecurityConfig(conf));
+
+ // Revoking a valid certificate should return true.
+ assertTrue(revoked.get());
+
Review comment:
can we verify that after revoke, getCertificateByID from ca with valid
and invalid type, respectively?
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/metadata/SCMDBDefinition.java
##########
@@ -73,13 +75,30 @@
public static final DBColumnFamilyDefinition<ContainerID, ContainerInfo>
CONTAINERS =
- new DBColumnFamilyDefinition<ContainerID, ContainerInfo>(
+ new DBColumnFamilyDefinition<>(
"containers",
ContainerID.class,
new ContainerIDCodec(),
ContainerInfo.class,
new ContainerInfoCodec());
+ public static final DBColumnFamilyDefinition<Long, CRLInfo> CRL_INFO =
+ new DBColumnFamilyDefinition<>(
+ "crlInfo",
+ Long.class,
+ new LongCodec(),
+ CRLInfo.class,
+ new CRLInfoCodec());
+
+ public static final DBColumnFamilyDefinition<String, Long>
+ CRL_SEQUENCE_ID =
Review comment:
It's a bit overkill to use a table to just save the crl sequence id. Any
chance to put it into the SCM version file?
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/metadata/SCMDBDefinition.java
##########
@@ -73,13 +75,30 @@
public static final DBColumnFamilyDefinition<ContainerID, ContainerInfo>
CONTAINERS =
- new DBColumnFamilyDefinition<ContainerID, ContainerInfo>(
+ new DBColumnFamilyDefinition<>(
"containers",
ContainerID.class,
new ContainerIDCodec(),
ContainerInfo.class,
new ContainerInfoCodec());
+ public static final DBColumnFamilyDefinition<Long, CRLInfo> CRL_INFO =
Review comment:
NIT: CRL_INFO=>CRLS to be consistent with other table definition.
##########
File path:
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/server/TestSCMCertStore.java
##########
@@ -0,0 +1,235 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.scm.server;
+
+import org.apache.hadoop.hdds.HddsConfigKeys;
+import org.apache.hadoop.hdds.conf.OzoneConfiguration;
+import org.apache.hadoop.hdds.scm.metadata.SCMMetadataStore;
+import org.apache.hadoop.hdds.scm.metadata.SCMMetadataStoreImpl;
+import org.apache.hadoop.hdds.security.x509.SecurityConfig;
+import
org.apache.hadoop.hdds.security.x509.certificate.authority.CertificateStore;
+import org.apache.hadoop.hdds.security.x509.certificate.utils.CertificateCodec;
+import org.apache.hadoop.hdds.security.x509.crl.CRLInfo;
+import org.apache.hadoop.hdds.utils.db.Table;
+import org.apache.hadoop.hdds.utils.db.TableIterator;
+import org.apache.hadoop.security.ssl.KeyStoreTestUtil;
+import org.bouncycastle.asn1.x509.CRLReason;
+import org.bouncycastle.cert.X509CertificateHolder;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+import java.io.IOException;
+import java.math.BigInteger;
+import java.nio.file.Files;
+import java.security.KeyPair;
+import java.security.cert.X509CRLEntry;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Set;
+
+import static org.apache.hadoop.ozone.OzoneConsts.CRL_SEQUENCE_ID_KEY;
+
+/**
+ * Test class for @{@link SCMCertStore}.
+ */
+public class TestSCMCertStore {
+
+ private static final String COMPONENT_NAME = "scm";
+ private static final Long INITIAL_SEQUENCE_ID = 1L;
+
+ private OzoneConfiguration config;
+ private SCMMetadataStore scmMetadataStore;
+ private SCMCertStore scmCertStore;
+ private SecurityConfig securityConfig;
+ private X509Certificate x509Certificate;
+ private KeyPair keyPair;
+
+ @Rule
+ public final TemporaryFolder tempDir = new TemporaryFolder();
+
+ @Before
+ public void setUp() throws Exception {
+ config = new OzoneConfiguration();
+
+ config.set(HddsConfigKeys.OZONE_METADATA_DIRS,
+ tempDir.newFolder().getAbsolutePath());
+
+ securityConfig = new SecurityConfig(config);
+ }
+
+ @Before
+ public void initDbStore() throws IOException {
+ scmMetadataStore = new SCMMetadataStoreImpl(config);
+ scmCertStore = new SCMCertStore(scmMetadataStore, INITIAL_SEQUENCE_ID);
+ }
+
+ @Before
+ public void generateCertificate() throws Exception {
+ Files.createDirectories(securityConfig.getKeyLocation(COMPONENT_NAME));
+ x509Certificate = generateX509Cert(null);
+ }
+
+ @After
+ public void destroyDbStore() throws Exception {
+ if (scmMetadataStore.getStore() != null) {
+ scmMetadataStore.getStore().close();
+ }
+ }
+
+ @Test
+ public void testRevokeCertificates() throws Exception {
+
+ BigInteger serialID = x509Certificate.getSerialNumber();
+ scmCertStore.storeValidCertificate(serialID, x509Certificate);
+
+ Assert.assertNotNull(
+ scmCertStore.getCertificateByID(serialID,
+ CertificateStore.CertType.VALID_CERTS));
+
+ X509CertificateHolder caCertificateHolder =
+ new X509CertificateHolder(generateX509Cert(keyPair).getEncoded());
+ List<X509Certificate> certs = new ArrayList<>();
+ certs.add(x509Certificate);
+ scmCertStore.revokeCertificates(certs,
+ caCertificateHolder,
+ CRLReason.unspecified, securityConfig,
+ keyPair);
+
+ Assert.assertNull(
+ scmCertStore.getCertificateByID(serialID,
+ CertificateStore.CertType.VALID_CERTS));
+
+ Assert.assertNotNull(
+ scmCertStore.getCertificateByID(serialID,
+ CertificateStore.CertType.REVOKED_CERTS));
+
+ // CRL Info table should have a CRL with sequence id
+ Assert.assertEquals(
+ INITIAL_SEQUENCE_ID + 1L,
+ (long) scmMetadataStore.getCRLInfoTable().iterator().next().getKey());
+
+ // Check the sequence ID table for latest sequence id
+ Assert.assertEquals(INITIAL_SEQUENCE_ID + 1L, (long)
+ scmMetadataStore.getCRLSequenceIdTable().get(CRL_SEQUENCE_ID_KEY));
+
+ CRLInfo crlInfo =
+ scmMetadataStore.getCRLInfoTable().iterator().next().getValue();
+
+ Set<? extends X509CRLEntry> revokedCertificates =
+ crlInfo.getX509CRL().getRevokedCertificates();
+ Assert.assertEquals(1L, revokedCertificates.size());
+ Assert.assertEquals(x509Certificate.getSerialNumber(),
+ revokedCertificates.iterator().next().getSerialNumber());
+
+ // Now trying to revoke the already revoked certificate should result in
+ // a warning message and no-op. It should not create a new CRL.
+ scmCertStore.revokeCertificates(certs,
Review comment:
How does the caller know that has already been revoked? a warning
message won't work at API level.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
