You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "Galsza (via GitHub)" <gi...@apache.org> on 2023/02/01 11:20:16 UTC
[GitHub] [ozone] Galsza opened a new pull request, #4231: HDDS-7379. Use certificate bundles instead of the sole certificate
Galsza opened a new pull request, #4231:
URL: https://github.com/apache/ozone/pull/4231
## What changes were proposed in this pull request?
Instead of using the sole certificate the whole cert bundle is used now.
In this new version, certificates are stored along with their entire certificate path up to the root CA. When getting these certificates, the whole chain is read back instead. In protocol messages the chain is converted into a String, so in reality `SCMGetCertResponseProto.x509Certificate` is now a pem encoded full certification chain.
Some minor refactors in CertificateCodec and removing some dead code is also included.
## What is the link to the Apache JIRA
[HDDS-7379](https://issues.apache.org/jira/browse/HDDS-7379)
## How was this patch tested?
Some local tests were added as well as a sanity check of running a local cluster with security enabled and inserting a key.
##Work in progress:
- Adding more tests
- Fixing an issue in CertificateCodec with the comment //Bug here, which might cause some services to not read the full CertificateChain
- Double check that getting the certificate chain is properly done in every place where a proto message is being read/written.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] fapifta commented on a diff in pull request #4231: HDDS-7379. Use certificate bundles instead of the sole certificate
Posted by "fapifta (via GitHub)" <gi...@apache.org>.
fapifta commented on code in PR #4231:
URL: https://github.com/apache/ozone/pull/4231#discussion_r1103379009
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/x509/CertificateClientTest.java:
##########
@@ -264,7 +279,8 @@ public void renewKey() throws Exception {
"CN=OzoneMaster", keyPair, 30, "SHA256withRSA");
keyPair = newKeyPair;
- x509Certificate = newCert;
+ CertificateFactory fact = CertificateFactory.getInstance("X.509");
Review Comment:
This and the next line repeatedly appear in two forms:
Based on java.security.cert.CertificateFactory this way:
```
CertificateFactory fact = CertificateFactory.getInstance("X.509");
certPath = fact.generateCertPath(ImmutableList.of(someCert));
```
and
Based on the BouncyCastle Certificate factory this way:
```
CertificateFactory factory = new CertificateFactory();
return factory.engineGenerateCertPath(updatedList);
```
We should I believe extract this to a method and use the same CertificateFactory for our purposes. I am not sure what is the difference though, so if it is justified I am fine with using both approach in parallel, if this is the case, please let me know the justification please. This same code appears in production and test code as well, similarly to getTargetCertificate/firstCertificateFrom. We might put these into CertificateCodec or a utility class besides the CertificateCodec, so we can cover all usages of these two methods all over the code.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java:
##########
@@ -800,9 +826,13 @@ public synchronized InitResponse init() throws CertificateException {
return handleCase(init);
}
+ private X509Certificate getTargetCertificate(CertPath certificatePath) {
Review Comment:
This method should be utilized in more places within this class, there are a couple occurrences where the same logic is there, that can be replaced with the method call.
Also what do you think about calling it firstCertificateFrom?
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java:
##########
@@ -292,22 +291,31 @@ public PublicKey getPublicKey() {
try {
publicKey = keyCodec.readPublicKey();
} catch (InvalidKeySpecException | NoSuchAlgorithmException
- | IOException e) {
+ | IOException e) {
Review Comment:
I don't think this indentation change is useful. There is no clear rule on this I could find quickly, but based on what we usually do with function parameters, I think the right indentation is 4 spaces here instead of 8.
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/x509/certificate/authority/TestDefaultCAServer.java:
##########
@@ -197,11 +200,14 @@ public void testRequestCertificate() throws IOException,
testCA.init(new SecurityConfig(conf),
SELF_SIGNED_CA);
- Future<X509CertificateHolder> holder = testCA.requestCertificate(csrString,
- CertificateApprover.ApprovalType.TESTING_AUTOMATIC, SCM);
+ Future<CertPath> holder = testCA.requestCertificate(
+ csrString, CertificateApprover.ApprovalType.TESTING_AUTOMATIC, SCM);
// Right now our calls are synchronous. Eventually this will have to wait.
assertTrue(holder.isDone());
- assertNotNull(holder.get());
+ //Test that the cert path returned contains the CA certificate in proper
+ // place
+ assertEquals(holder.get().getCertificates().get(1),
Review Comment:
Shouldn't we assert as well that the returned certificate at the first place matches the data that we specified in the CSR?
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/x509/certificate/utils/TestCertificateCodec.java:
##########
@@ -33,27 +35,30 @@
import java.nio.file.Path;
import java.security.NoSuchAlgorithmException;
import java.security.NoSuchProviderException;
+import java.security.cert.CertPath;
+import java.security.cert.Certificate;
import java.security.cert.CertificateException;
import java.security.cert.X509Certificate;
import java.time.LocalDateTime;
import static org.apache.hadoop.hdds.HddsConfigKeys.OZONE_METADATA_DIRS;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
/**
* Tests the Certificate codecs.
*/
public class TestCertificateCodec {
- private static OzoneConfiguration conf = new OzoneConfiguration();
+ private static final OzoneConfiguration CONF = new OzoneConfiguration();
Review Comment:
I am not sure if this is the right direction...
If you look at the usage of the conf instance, at the moment, none of the tests use it for anything, but that might change later, and in that case having a static final reference may have unintended side effects.
As the conf is not needed in any static context, I would move it to the instance level by removing the static keyword instead of making it final, and would create a new instance for every test in the init method marked with the @BeforeEach annotation to run before all tests. What do you think about this approach?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/HASecurityUtils.java:
##########
@@ -192,24 +192,26 @@ private static void getPrimarySCMSelfSignedCert(CertificateClient client,
PKCS10CertificationRequest csr = generateCSR(client, scmStorageConfig,
config, scmAddress);
- X509CertificateHolder subSCMCertHolder = rootCAServer.
+ CertPath subSCMCertHolderList = rootCAServer.
requestCertificate(csr, KERBEROS_TRUSTED, SCM).get();
- X509CertificateHolder rootCACertificateHolder =
- rootCAServer.getCACertificate();
+ CertPath rootCACertificatePath =
+ rootCAServer.getCaCertPath();
String pemEncodedCert =
- CertificateCodec.getPEMEncodedString(subSCMCertHolder);
-
+ CertificateCodec.getPEMEncodedString(subSCMCertHolderList);
+
Review Comment:
Can we remove these spaces please?
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/x509/certificate/authority/TestDefaultCAServer.java:
##########
@@ -197,11 +200,14 @@ public void testRequestCertificate() throws IOException,
testCA.init(new SecurityConfig(conf),
SELF_SIGNED_CA);
- Future<X509CertificateHolder> holder = testCA.requestCertificate(csrString,
- CertificateApprover.ApprovalType.TESTING_AUTOMATIC, SCM);
+ Future<CertPath> holder = testCA.requestCertificate(
+ csrString, CertificateApprover.ApprovalType.TESTING_AUTOMATIC, SCM);
// Right now our calls are synchronous. Eventually this will have to wait.
assertTrue(holder.isDone());
- assertNotNull(holder.get());
+ //Test that the cert path returned contains the CA certificate in proper
+ // place
+ assertEquals(holder.get().getCertificates().get(1),
Review Comment:
Shouldn't we assert as well that the returned certificate at the first place matches the data that we specified in the CSR?
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/CertificateClient.java:
##########
@@ -365,7 +379,17 @@ default void assertValidKeysAndCertificate() throws OzoneSecurityException {
/**
* Register a receiver that will be called after the certificate renewed.
+ *
* @param receiver
*/
void registerNotificationReceiver(CertificateNotification receiver);
+
+ /**
+ * Type for specifying the type of the certificate to be stored.
+ */
+ enum CertType {
Review Comment:
What if we add a fileNamePrefix to the enum, and a getter for that, so we can use polymorphism later on in the storeCertificate method, where we currently have an if statement to determine the prefix?
On the other hand, the name of this enum and its values are a bit problematic I believe. There is an other CertType enum used in the CertificateStore class that is used by the CA server to store the certificates it signs, I see a potential ambiguity between the two, and so that made me think of a better name for this.
The real role of this seems to be to provide a ternary logic construct to decide if a certificate is CA and if CA then it is a ROOT_CA certificate. So an idea might be to change this to something like CAType, and have values like SUBORDINATE, ROOT, and NONE.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java:
##########
@@ -601,58 +621,64 @@ public X509Certificate queryCertificate(String query) {
* Stores the Certificate for this client. Don't use this api to add trusted
* certificates of others.
*
- * @param pemEncodedCert - pem encoded X509 Certificate
- * @param force - override any existing file
+ * @param pemEncodedCert - pem encoded X509 Certificate
* @throws CertificateException - on Error.
- *
*/
@Override
- public void storeCertificate(String pemEncodedCert, boolean force)
+ public void storeCertificate(String pemEncodedCert)
throws CertificateException {
- this.storeCertificate(pemEncodedCert, force, false);
+ this.storeCertificate(pemEncodedCert, false);
}
/**
* Stores the Certificate for this client. Don't use this api to add trusted
* certificates of others.
*
- * @param pemEncodedCert - pem encoded X509 Certificate
- * @param force - override any existing file
- * @param caCert - Is CA certificate.
+ * @param pemEncodedCert - pem encoded X509 Certificate
+ * @param caCert - Is CA certificate.
* @throws CertificateException - on Error.
- *
*/
@Override
- public void storeCertificate(String pemEncodedCert, boolean force,
+ public void storeCertificate(String pemEncodedCert,
Review Comment:
Shouldn't we use the CertType here as well to define if the certificate to be stored is a CA cert or not?
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/x509/certificate/utils/TestCertificateCodec.java:
##########
@@ -104,6 +94,50 @@ public void testGetPEMEncodedString()
assertEquals(firstCert, secondCert);
}
+ /**
+ * Test when converting a certificate path to pem encoded string and back
+ * we get back the same certificates.
+ */
+ @Test
+ public void testGetPemEncodedStringFromCertPath() throws IOException,
+ NoSuchAlgorithmException, NoSuchProviderException, CertificateException {
+ X509CertificateHolder certHolder1 = generateTestCert();
+ X509CertificateHolder certHolder2 = generateTestCert();
+ X509Certificate cert1 = CertificateCodec.getX509Certificate(certHolder1);
+ X509Certificate cert2 = CertificateCodec.getX509Certificate(certHolder2);
+ CertificateFactory certificateFactory = new CertificateFactory();
+ CertPath pathToEncode =
+ certificateFactory.engineGenerateCertPath(ImmutableList.of(cert1,
+ cert2));
+ String encodedPath = CertificateCodec.getPEMEncodedString(pathToEncode);
Review Comment:
Just a thought, in these tests, not just this one, in all of these tests, we are testing the CertificateCodec class in a way that we generate a certificate object, use the codec to re-write the certificate into PEM format, and then we are reading back the PEM format string as a certificate object, and finally we are checking for equality.
There is a potential flaw here, namely if the codec itself is able to generate a PEM format string, and read it back, but nothing else can read that PEM format string.
Our codec relies on underlying libraries that we do not want to test, but I think we should at least have one test that ensures that the PEM file we are writing out is readable with the standard openssl tool.
Please let me know if you agree, and if so, let's create a note/ticket for us to review these tests, and handle this later on in a separate PR.
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/x509/certificate/client/TestDefaultCertificateClient.java:
##########
@@ -506,6 +506,7 @@ public void testRenewAndStoreKeyAndCertificate() throws Exception {
.newBuilder().setResponseCode(SCMSecurityProtocolProtos
.SCMGetCertResponseProto.ResponseCode.success)
.setX509Certificate(pemCert)
+ .setX509Certificate(pemCert)
Review Comment:
I think this one is either a typo or an accident... :) Can you please take a look what was your intent here?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/security/OMCertificateClient.java:
##########
@@ -162,26 +164,26 @@ public CertificateSignRequest.Builder getCSRBuilder(KeyPair keyPair)
@Override
public String signAndStoreCertificate(PKCS10CertificationRequest request,
- Path certPath) throws CertificateException {
+ Path certificatePath) throws CertificateException {
try {
SCMGetCertResponseProto response = getScmSecureClient()
.getOMCertChain(omInfo, getEncodedString(request));
String pemEncodedCert = response.getX509Certificate();
CertificateCodec certCodec = new CertificateCodec(
- getSecurityConfig(), certPath);
+ getSecurityConfig(), certificatePath);
// Store SCM CA certificate.
if (response.hasX509CACertificate()) {
String pemEncodedRootCert = response.getX509CACertificate();
storeCertificate(pemEncodedRootCert,
- true, true, false, certCodec, false);
- storeCertificate(pemEncodedCert, true, false, false, certCodec, false);
+ CA, certCodec, false);
+ storeCertificate(pemEncodedCert, INTERMEDIATE, certCodec, false);
// Store Root CA certificate if available.
if (response.hasX509RootCACertificate()) {
storeCertificate(response.getX509RootCACertificate(),
- true, false, true, certCodec, false);
+ CertType.ROOT_CA, certCodec, false);
Review Comment:
This one I believe also happens in a few places, where you have statically imported the INTERMEDIATE CertType, while later you reference the ROOT_CA type with the classname classified. Can you please unify these as well, and either use CertType.INTERMEDIATE similarly to CertType.ROOT_CA, or import statically the ROOT_CA value as well?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] Galsza commented on a diff in pull request #4231: HDDS-7379. Use certificate bundles instead of the sole certificate
Posted by "Galsza (via GitHub)" <gi...@apache.org>.
Galsza commented on code in PR #4231:
URL: https://github.com/apache/ozone/pull/4231#discussion_r1108614166
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/security/OMCertificateClient.java:
##########
@@ -162,26 +164,26 @@ public CertificateSignRequest.Builder getCSRBuilder(KeyPair keyPair)
@Override
public String signAndStoreCertificate(PKCS10CertificationRequest request,
- Path certPath) throws CertificateException {
+ Path certificatePath) throws CertificateException {
try {
SCMGetCertResponseProto response = getScmSecureClient()
.getOMCertChain(omInfo, getEncodedString(request));
String pemEncodedCert = response.getX509Certificate();
CertificateCodec certCodec = new CertificateCodec(
- getSecurityConfig(), certPath);
+ getSecurityConfig(), certificatePath);
// Store SCM CA certificate.
if (response.hasX509CACertificate()) {
String pemEncodedRootCert = response.getX509CACertificate();
storeCertificate(pemEncodedRootCert,
- true, true, false, certCodec, false);
- storeCertificate(pemEncodedCert, true, false, false, certCodec, false);
+ CA, certCodec, false);
+ storeCertificate(pemEncodedCert, INTERMEDIATE, certCodec, false);
// Store Root CA certificate if available.
if (response.hasX509RootCACertificate()) {
storeCertificate(response.getX509RootCACertificate(),
- true, false, true, certCodec, false);
+ CertType.ROOT_CA, certCodec, false);
Review Comment:
They should now be consistent.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] Galsza commented on a diff in pull request #4231: HDDS-7379. Use certificate bundles instead of the sole certificate
Posted by "Galsza (via GitHub)" <gi...@apache.org>.
Galsza commented on code in PR #4231:
URL: https://github.com/apache/ozone/pull/4231#discussion_r1108613414
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/x509/certificate/utils/TestCertificateCodec.java:
##########
@@ -33,27 +35,30 @@
import java.nio.file.Path;
import java.security.NoSuchAlgorithmException;
import java.security.NoSuchProviderException;
+import java.security.cert.CertPath;
+import java.security.cert.Certificate;
import java.security.cert.CertificateException;
import java.security.cert.X509Certificate;
import java.time.LocalDateTime;
import static org.apache.hadoop.hdds.HddsConfigKeys.OZONE_METADATA_DIRS;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
/**
* Tests the Certificate codecs.
*/
public class TestCertificateCodec {
- private static OzoneConfiguration conf = new OzoneConfiguration();
+ private static final OzoneConfiguration CONF = new OzoneConfiguration();
Review Comment:
Thanks. Yours is a better direction definitely.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] Galsza commented on a diff in pull request #4231: HDDS-7379. Use certificate bundles instead of the sole certificate
Posted by "Galsza (via GitHub)" <gi...@apache.org>.
Galsza commented on code in PR #4231:
URL: https://github.com/apache/ozone/pull/4231#discussion_r1108597666
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java:
##########
@@ -601,58 +621,64 @@ public X509Certificate queryCertificate(String query) {
* Stores the Certificate for this client. Don't use this api to add trusted
* certificates of others.
*
- * @param pemEncodedCert - pem encoded X509 Certificate
- * @param force - override any existing file
+ * @param pemEncodedCert - pem encoded X509 Certificate
* @throws CertificateException - on Error.
- *
*/
@Override
- public void storeCertificate(String pemEncodedCert, boolean force)
+ public void storeCertificate(String pemEncodedCert)
throws CertificateException {
- this.storeCertificate(pemEncodedCert, force, false);
+ this.storeCertificate(pemEncodedCert, false);
}
/**
* Stores the Certificate for this client. Don't use this api to add trusted
* certificates of others.
*
- * @param pemEncodedCert - pem encoded X509 Certificate
- * @param force - override any existing file
- * @param caCert - Is CA certificate.
+ * @param pemEncodedCert - pem encoded X509 Certificate
+ * @param caCert - Is CA certificate.
* @throws CertificateException - on Error.
- *
*/
@Override
- public void storeCertificate(String pemEncodedCert, boolean force,
+ public void storeCertificate(String pemEncodedCert,
Review Comment:
Done. CAType is used now and it's also a unified enum from CertificateClient and CertificateServer
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] fapifta commented on pull request #4231: HDDS-7379. Use certificate bundles instead of the sole certificate
Posted by "fapifta (via GitHub)" <gi...@apache.org>.
fapifta commented on PR #4231:
URL: https://github.com/apache/ozone/pull/4231#issuecomment-1433737486
Thank you for the continued work on this one @Galsza. I went through the changes that you added after my comments so far, and it seems to be good. I would like to scan through the changes together with all the commits just to be sure I checked all changes together as well.
In the meantime I have restarted the failing CI test run.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] Galsza commented on a diff in pull request #4231: HDDS-7379. Use certificate bundles instead of the sole certificate
Posted by "Galsza (via GitHub)" <gi...@apache.org>.
Galsza commented on code in PR #4231:
URL: https://github.com/apache/ozone/pull/4231#discussion_r1108632616
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/x509/certificate/utils/TestCertificateCodec.java:
##########
@@ -104,6 +94,50 @@ public void testGetPEMEncodedString()
assertEquals(firstCert, secondCert);
}
+ /**
+ * Test when converting a certificate path to pem encoded string and back
+ * we get back the same certificates.
+ */
+ @Test
+ public void testGetPemEncodedStringFromCertPath() throws IOException,
+ NoSuchAlgorithmException, NoSuchProviderException, CertificateException {
+ X509CertificateHolder certHolder1 = generateTestCert();
+ X509CertificateHolder certHolder2 = generateTestCert();
+ X509Certificate cert1 = CertificateCodec.getX509Certificate(certHolder1);
+ X509Certificate cert2 = CertificateCodec.getX509Certificate(certHolder2);
+ CertificateFactory certificateFactory = new CertificateFactory();
+ CertPath pathToEncode =
+ certificateFactory.engineGenerateCertPath(ImmutableList.of(cert1,
+ cert2));
+ String encodedPath = CertificateCodec.getPEMEncodedString(pathToEncode);
Review Comment:
Created a sub task for this issue: [HDDS-7981](https://issues.apache.org/jira/browse/HDDS-7981)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] Galsza commented on a diff in pull request #4231: HDDS-7379. Use certificate bundles instead of the sole certificate
Posted by "Galsza (via GitHub)" <gi...@apache.org>.
Galsza commented on code in PR #4231:
URL: https://github.com/apache/ozone/pull/4231#discussion_r1108615999
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/x509/certificate/authority/TestDefaultCAServer.java:
##########
@@ -197,11 +200,14 @@ public void testRequestCertificate() throws IOException,
testCA.init(new SecurityConfig(conf),
SELF_SIGNED_CA);
- Future<X509CertificateHolder> holder = testCA.requestCertificate(csrString,
- CertificateApprover.ApprovalType.TESTING_AUTOMATIC, SCM);
+ Future<CertPath> holder = testCA.requestCertificate(
+ csrString, CertificateApprover.ApprovalType.TESTING_AUTOMATIC, SCM);
// Right now our calls are synchronous. Eventually this will have to wait.
assertTrue(holder.isDone());
- assertNotNull(holder.get());
+ //Test that the cert path returned contains the CA certificate in proper
+ // place
+ assertEquals(holder.get().getCertificates().get(1),
Review Comment:
This is a repeated comment, see my answer in the other one.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] Galsza commented on a diff in pull request #4231: HDDS-7379. Use certificate bundles instead of the sole certificate
Posted by "Galsza (via GitHub)" <gi...@apache.org>.
Galsza commented on code in PR #4231:
URL: https://github.com/apache/ozone/pull/4231#discussion_r1108612741
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/x509/certificate/client/TestDefaultCertificateClient.java:
##########
@@ -506,6 +506,7 @@ public void testRenewAndStoreKeyAndCertificate() throws Exception {
.newBuilder().setResponseCode(SCMSecurityProtocolProtos
.SCMGetCertResponseProto.ResponseCode.success)
.setX509Certificate(pemCert)
+ .setX509Certificate(pemCert)
Review Comment:
Accident.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] fapifta merged pull request #4231: HDDS-7379. Use certificate bundles instead of the sole certificate
Posted by "fapifta (via GitHub)" <gi...@apache.org>.
fapifta merged PR #4231:
URL: https://github.com/apache/ozone/pull/4231
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] Galsza commented on a diff in pull request #4231: HDDS-7379. Use certificate bundles instead of the sole certificate
Posted by "Galsza (via GitHub)" <gi...@apache.org>.
Galsza commented on code in PR #4231:
URL: https://github.com/apache/ozone/pull/4231#discussion_r1108606300
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/CertificateClient.java:
##########
@@ -365,7 +379,17 @@ default void assertValidKeysAndCertificate() throws OzoneSecurityException {
/**
* Register a receiver that will be called after the certificate renewed.
+ *
* @param receiver
*/
void registerNotificationReceiver(CertificateNotification receiver);
+
+ /**
+ * Type for specifying the type of the certificate to be stored.
+ */
+ enum CertType {
Review Comment:
This is a big refactor. I've done it. CAType also existed in CertificateServer, now they are unified in a class level package level enum
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] Galsza commented on a diff in pull request #4231: HDDS-7379. Use certificate bundles instead of the sole certificate
Posted by "Galsza (via GitHub)" <gi...@apache.org>.
Galsza commented on code in PR #4231:
URL: https://github.com/apache/ozone/pull/4231#discussion_r1108612331
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/x509/certificate/authority/TestDefaultCAServer.java:
##########
@@ -197,11 +200,14 @@ public void testRequestCertificate() throws IOException,
testCA.init(new SecurityConfig(conf),
SELF_SIGNED_CA);
- Future<X509CertificateHolder> holder = testCA.requestCertificate(csrString,
- CertificateApprover.ApprovalType.TESTING_AUTOMATIC, SCM);
+ Future<CertPath> holder = testCA.requestCertificate(
+ csrString, CertificateApprover.ApprovalType.TESTING_AUTOMATIC, SCM);
// Right now our calls are synchronous. Eventually this will have to wait.
assertTrue(holder.isDone());
- assertNotNull(holder.get());
+ //Test that the cert path returned contains the CA certificate in proper
+ // place
+ assertEquals(holder.get().getCertificates().get(1),
Review Comment:
I've added a check to assert for the CA being the signer of the resulting certificate. I'm not sure if there is a better way of doing this, let me know if you have a suggestion.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] Galsza commented on a diff in pull request #4231: HDDS-7379. Use certificate bundles instead of the sole certificate
Posted by "Galsza (via GitHub)" <gi...@apache.org>.
Galsza commented on code in PR #4231:
URL: https://github.com/apache/ozone/pull/4231#discussion_r1108608728
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/x509/CertificateClientTest.java:
##########
@@ -264,7 +279,8 @@ public void renewKey() throws Exception {
"CN=OzoneMaster", keyPair, 30, "SHA256withRSA");
keyPair = newKeyPair;
- x509Certificate = newCert;
+ CertificateFactory fact = CertificateFactory.getInstance("X.509");
Review Comment:
Done. Moved into CertificateCodec.
(In the end I chose BouncyCastle, it rearranges certificates to be in order when creating a cert path and it is also able to read pem files seamlessly.)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] fapifta commented on a diff in pull request #4231: HDDS-7379. Use certificate bundles instead of the sole certificate
Posted by "fapifta (via GitHub)" <gi...@apache.org>.
fapifta commented on code in PR #4231:
URL: https://github.com/apache/ozone/pull/4231#discussion_r1109030723
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/x509/certificate/authority/TestDefaultCAServer.java:
##########
@@ -197,11 +200,14 @@ public void testRequestCertificate() throws IOException,
testCA.init(new SecurityConfig(conf),
SELF_SIGNED_CA);
- Future<X509CertificateHolder> holder = testCA.requestCertificate(csrString,
- CertificateApprover.ApprovalType.TESTING_AUTOMATIC, SCM);
+ Future<CertPath> holder = testCA.requestCertificate(
+ csrString, CertificateApprover.ApprovalType.TESTING_AUTOMATIC, SCM);
// Right now our calls are synchronous. Eventually this will have to wait.
assertTrue(holder.isDone());
- assertNotNull(holder.get());
+ //Test that the cert path returned contains the CA certificate in proper
+ // place
+ assertEquals(holder.get().getCertificates().get(1),
Review Comment:
Looks good.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org