You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/07/20 14:18:49 UTC
[GitHub] [geode] jdeppe-pivotal opened a new pull request #6707: GEODE-9439: Convert CertificateBuilder to sun/jdk APIs
jdeppe-pivotal opened a new pull request #6707:
URL: https://github.com/apache/geode/pull/6707
- Makes it easier to de/serialize created certificates
- Remove the requirement for the Bouncy Castle library
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp commented on pull request #6707: GEODE-9439: Convert CertificateBuilder to sun/jdk APIs
Posted by GitBox <gi...@apache.org>.
mhansonp commented on pull request #6707:
URL: https://github.com/apache/geode/pull/6707#issuecomment-883718201
> It's a change to a test helper class - I didn't think it would warrant any significant discussion.
Sounds good. Approved..
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp commented on pull request #6707: GEODE-9439: Convert CertificateBuilder to sun/jdk APIs
Posted by GitBox <gi...@apache.org>.
mhansonp commented on pull request #6707:
URL: https://github.com/apache/geode/pull/6707#issuecomment-883652223
I don't remember seeing this as a proprosal in the dev channel. Is this something that should be there? I am just asking the question... I am curious if this affects Spring or others.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] Bill commented on a change in pull request #6707: GEODE-9439: Convert CertificateBuilder to sun/jdk APIs
Posted by GitBox <gi...@apache.org>.
Bill commented on a change in pull request #6707:
URL: https://github.com/apache/geode/pull/6707#discussion_r673479246
##########
File path: geode-junit/src/main/java/org/apache/geode/cache/ssl/CertificateBuilder.java
##########
@@ -79,16 +77,27 @@ public CertificateBuilder(int days, String algorithm) {
}
private static GeneralName dnsGeneralName(String name) {
- return new GeneralName(GeneralName.dNSName, name);
+ try {
+ return new GeneralName(new DNSName(name));
+ } catch (IOException ex) {
+ throw new UncheckedIOException(ex);
+ }
Review comment:
The value of bundling this as into a runtime exception is that this method can still be used in contexts (e.g. Stream.map(f)`) where a method throwing a checked exception would not be allowed. Ok.
##########
File path: geode-junit/build.gradle
##########
@@ -19,6 +19,10 @@ apply from: "${rootDir}/${scriptDir}/standard-subproject-configuration.gradle"
apply from: "${project.projectDir}/../gradle/publish-java.gradle"
+compileJava {
+ options.compilerArgs << '-Xlint:-sunapi'
+ options.compilerArgs << '-XDenableSunApiLintControl'
Review comment:
So we've moved from Bouncy Castle, a high quality open source project, to an internal JDK class. These compiler options suppress what would be compile errors I suppose (or at least warnings) that we are illegally accessing JDK internal classes.
My research shows that in JDK 9 these classes in a `sun` package will move to a `javax` one. I suppose when that happens the JDK8 internal classes accessed here will have direct replacements available so all we'll have to do is change the includes in these files.
##########
File path: geode-junit/src/main/java/org/apache/geode/cache/ssl/CertificateBuilder.java
##########
@@ -146,58 +164,69 @@ public CertificateMaterial generate() {
}
private X509Certificate generate(PublicKey publicKey, PrivateKey privateKey) {
+ Date from = new Date();
+ Date to = new Date(from.getTime() + days * 86_400_000L);
+
+ CertificateValidity interval = new CertificateValidity(from, to);
+ BigInteger sn = new BigInteger(64, new SecureRandom());
+
+ X509CertInfo info = new X509CertInfo();
+
try {
- AlgorithmIdentifier sigAlgId =
- new DefaultSignatureAlgorithmIdentifierFinder().find(algorithm);
- AlgorithmIdentifier digAlgId = new DefaultDigestAlgorithmIdentifierFinder().find(sigAlgId);
- AsymmetricKeyParameter publicKeyAsymKeyParam =
- PublicKeyFactory.createKey(publicKey.getEncoded());
- AsymmetricKeyParameter privateKeyAsymKeyParam =
- PrivateKeyFactory.createKey(privateKey.getEncoded());
- SubjectPublicKeyInfo subPubKeyInfo =
- SubjectPublicKeyInfo.getInstance(publicKey.getEncoded());
-
- ContentSigner sigGen =
- new BcRSAContentSignerBuilder(sigAlgId, digAlgId).build(privateKeyAsymKeyParam);
-
- Date from = new Date();
- Date to = new Date(from.getTime() + days * 86400000L);
- BigInteger sn = new BigInteger(64, new SecureRandom());
-
- X509v3CertificateBuilder v3CertGen;
+ info.set(X509CertInfo.VALIDITY, interval);
+ info.set(X509CertInfo.SERIAL_NUMBER, new CertificateSerialNumber(sn));
+ info.set(X509CertInfo.SUBJECT, name);
+ info.set(X509CertInfo.KEY, new CertificateX509Key(publicKey));
+ info.set(X509CertInfo.VERSION, new CertificateVersion(CertificateVersion.V3));
+ AlgorithmId algo = new AlgorithmId(AlgorithmId.md5WithRSAEncryption_oid);
+ info.set(X509CertInfo.ALGORITHM_ID, new CertificateAlgorithmId(algo));
+
if (issuer == null) {
// This is a self-signed certificate
- v3CertGen = new X509v3CertificateBuilder(name, sn, from, to, name, subPubKeyInfo);
+ info.set(X509CertInfo.ISSUER, name);
} else {
- v3CertGen = new X509v3CertificateBuilder(
- new X500Name(issuer.getCertificate().getIssuerDN().getName()),
- sn, from, to, name, subPubKeyInfo);
+ info.set(X509CertInfo.ISSUER, issuer.getCertificate().getSubjectDN());
}
- byte[] subjectAltName = san();
- if (subjectAltName != null) {
- v3CertGen.addExtension(Extension.subjectAlternativeName, false, subjectAltName);
+ CertificateExtensions extensions = new CertificateExtensions();
+
+ byte[] keyIdBytes = new KeyIdentifier(publicKey).getIdentifier();
+ SubjectKeyIdentifierExtension keyIdentifier = new SubjectKeyIdentifierExtension(keyIdBytes);
+ extensions.set(SubjectKeyIdentifierExtension.NAME, keyIdentifier);
+
+ GeneralNames subjectAltNames = san();
+ if (!subjectAltNames.isEmpty()) {
+ SubjectAlternativeNameExtension altNames =
+ new SubjectAlternativeNameExtension(subjectAltNames);
+ extensions.set(SubjectAlternativeNameExtension.NAME, altNames);
}
if (isCA) {
- v3CertGen.addExtension(Extension.keyUsage, true, new KeyUsage(KeyUsage.keyCertSign));
- v3CertGen.addExtension(Extension.basicConstraints, true, new BasicConstraints(0));
+ KeyUsageExtension usageExtension = new KeyUsageExtension();
+ usageExtension.set(KeyUsageExtension.KEY_CERTSIGN, true);
+ extensions.set(KeyUsageExtension.NAME, usageExtension);
+
+ BasicConstraintsExtension basicConstraints = new BasicConstraintsExtension(true, 0);
+ extensions.set(BasicConstraintsExtension.NAME, basicConstraints);
+ }
+
+ if (!extensions.getAllExtensions().isEmpty()) {
+ info.set(X509CertInfo.EXTENSIONS, extensions);
}
- // Not strictly necessary, but this makes the certs look like those generated
- // by `keytool`.
- SubjectKeyIdentifier subjectKeyIdentifier =
- new SubjectKeyIdentifier(
- SubjectPublicKeyInfoFactory.createSubjectPublicKeyInfo(publicKeyAsymKeyParam)
- .getEncoded());
- v3CertGen.addExtension(Extension.subjectKeyIdentifier, false, subjectKeyIdentifier);
-
- X509CertificateHolder certificateHolder = v3CertGen.build(sigGen);
- return new JcaX509CertificateConverter()
- .setProvider(new BouncyCastleProvider())
- .getCertificate(certificateHolder);
- } catch (Exception e) {
- throw new RuntimeException("Unable to create certificate", e);
+ // Sign the cert to identify the algorithm that's used.
+ X509CertImpl cert = new X509CertImpl(info);
+ cert.sign(privateKey, algorithm);
+
+ // Update the algorithm, and resign.
+ algo = (AlgorithmId) cert.get(X509CertImpl.SIG_ALG);
+ info.set(CertificateAlgorithmId.NAME + "." + CertificateAlgorithmId.ALGORITHM, algo);
+ cert = new X509CertImpl(info);
+ cert.sign(privateKey, algorithm);
+
+ return cert;
+ } catch (Exception ex) {
+ throw new RuntimeException("Unable to create certificate", ex);
Review comment:
I'm counting on our test suite to catch any regressions in this method.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #6707: GEODE-9439: Convert CertificateBuilder to sun/jdk APIs
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on pull request #6707:
URL: https://github.com/apache/geode/pull/6707#issuecomment-883719912
@ladyVader Oops - thanks for pointing that out. Just pushed that change.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #6707: GEODE-9439: Convert CertificateBuilder to sun/jdk APIs
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on pull request #6707:
URL: https://github.com/apache/geode/pull/6707#issuecomment-883658015
It's a change to a test helper class - I didn't think it would warrant any significant discussion.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] Bill commented on pull request #6707: GEODE-9439: Convert CertificateBuilder to sun/jdk APIs
Posted by GitBox <gi...@apache.org>.
Bill commented on pull request #6707:
URL: https://github.com/apache/geode/pull/6707#issuecomment-883547849
🥇
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal merged pull request #6707: GEODE-9439: Convert CertificateBuilder to sun/jdk APIs
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal merged pull request #6707:
URL: https://github.com/apache/geode/pull/6707
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org