Using full cert chain over single certificate #161

zendern · 2021-07-19T01:18:32Z

Fixes #156 and #73

Found that we were parsing the cert using
CertificateFactory.getCertificate. This was not giving us the full chain
but instead just the domain cert. Which in most places work but not in
all.

To address this we now will get the full chain using
CertificateFactor.getCertificates and pass that on the event to Netty so
that it can be used for initialization.

zendern · 2021-07-19T01:19:18Z

acme/src/main/java/io/micronaut/acme/services/AcmeService.java

+            CertificateFactory cf = CertificateFactory.getInstance("X.509");
+            File certificate = new File(certLocation, DOMAIN_CRT);
+            if (certificate.exists()) {
+                return cf.generateCertificates(Files.newInputStream(certificate.toPath())).toArray(new X509Certificate[0]);


Important change was to call cf.generateCertificates here and then pass this into the CertificateEvent and thus into Netty for initialization.

michaelkoetter · 2021-07-19T10:30:17Z

acme/src/main/java/io/micronaut/acme/events/CertificateEvent.java

@@ -24,6 +24,7 @@
 public class CertificateEvent {
    private final X509Certificate certificate;


Is this still required, ie. should there be a distinction between "single certificate" and "certificate chain"?
The "single certificate" code path is only used for tls-apln-01 validation, but this could also utilize the fullCertificateChain array (just with a single entry).

Yah thats fair. I left it due to the CertificateEvent constructor is public so in theory someone could be using it. But yes implementation wise I could just store the full chain and in the validation cert case an array of 1 instead of 2 objects.

yawkat · 2022-03-02T16:06:53Z

@zendern is there a reason this is still a draft? I would like to merge it, I see no obvious issues.

Found that we were parsing the cert using CertificateFactory.getCertificate. This was not giving us the full chain but instead just the domain cert. Which in most places work but not in all. To address this we now will get the full chain using CertificateFactor.getCertificates and pass that on the event to Netty so that it can be used for initalization.

zendern · 2022-03-07T00:15:13Z

@zendern is there a reason this is still a draft? I would like to merge it, I see no obvious issues.

After rebasing I remember now. CI tests were broken but worked locally. Getting into trying to figure out why. Hoping to get it figured out this week so we can get this merged

sdelamo

It looks good. A couple of minor things.

acme/src/main/java/io/micronaut/acme/events/CertificateEvent.java

acme/src/main/java/io/micronaut/acme/services/AcmeService.java

acme/src/test/groovy/io/micronaut/acme/AcmeCertRefresherMultiDomainsTaskSpec.groovy

acme/src/test/groovy/io/micronaut/acme/AcmeCertRefresherTaskSpec.groovy

.../test/groovy/io/micronaut/acme/challenges/AcmeCertRefresherTaskTlsApln01ChallengeSpec.groovy

...src/test/groovy/io/micronaut/acme/challenges/AcmeCertRefresherTaskHttp01ChallengeSpec.groovy

acme/src/test/groovy/io/micronaut/acme/AcmeCertWildcardRefresherTaskSpec.groovy

sdelamo · 2022-03-08T15:52:02Z

acme/src/main/java/io/micronaut/acme/services/AcmeService.java

@@ -278,7 +303,7 @@ private boolean writeCombinedFile(Certificate certificate) {
                    try (BufferedWriter writer = Files.newBufferedWriter(domainCsr.toPath(), WRITE, CREATE, TRUNCATE_EXISTING)) {
                        certificate.writeCertificate(writer);
                    }
-                    eventPublisher.publishEvent(new CertificateEvent(getCurrentCertificate(), domainKeyPair, false));
+                    eventPublisher.publishEvent(new CertificateEvent(domainKeyPair, false, getFullCertificateChain()));


if getFullCertificateChain returns an empty array, should we publish the event?. Since CertificateEvent assumes a non empty array due to CertificateEvent::getCert where it gets the first certificate in the array.

Resolved via 6a5dd44

sdelamo · 2022-03-08T15:52:16Z

acme/src/main/java/io/micronaut/acme/services/AcmeService.java

@@ -482,7 +507,7 @@ private void doChallengeSpecificSetup(Authorization auth, Challenge challenge) t
     * Setup the certificate that has been saved to disk and configures it for use.
     */
    public void setupCurrentCertificate() {
-        eventPublisher.publishEvent(new CertificateEvent(getCurrentCertificate(), getDomainKeyPair(), false));
+        eventPublisher.publishEvent(new CertificateEvent(getDomainKeyPair(), false, getFullCertificateChain()));


if getFullCertificateChain returns an empty array, should we publish the event?. Since CertificateEvent assumes a non empty array due to CertificateEvent::getCert where it gets the first certificate in the array.

Resolved via 6a5dd44

recursivecodes · 2022-03-11T22:27:13Z

Can we try this build again? I'd really love to have this merged soon.

yawkat · 2022-03-14T09:49:45Z

I think the sonar CI is just broken. we can ignore it for the merge

recursivecodes · 2022-03-16T15:57:01Z

OK - who can do the merge @yawkat?

yawkat · 2022-03-16T16:11:02Z

just waiting for sergio to re-review this. I will handle the rest.

sdelamo · 2022-03-16T17:52:41Z

thanks for the pr @zendern I have change the method signature to return optional 6a5dd44 to be on the safer side.

recursivecodes · 2022-03-17T12:41:08Z

I am receiving an exception now when trying 3.1.1.SNAPSHOT:

#211

recursivecodes · 2022-03-17T12:53:32Z

Same issue with MN-ACME 3.1.0 on Micronaut 3.2.0

zendern commented Jul 19, 2021

View reviewed changes

michaelkoetter reviewed Jul 19, 2021

View reviewed changes

yawkat self-assigned this Mar 3, 2022

zendern force-pushed the full-chain branch from dc952a7 to 2502bf0 Compare March 6, 2022 21:54

zendern added 3 commits March 6, 2022 17:22

No need to store another certificate just use the chain

3bfff20

No reason we cant deprecate the old constructor and use the new one

1c3fc8d

zendern force-pushed the full-chain branch from 2502bf0 to 1c3fc8d Compare March 6, 2022 22:30

Cleanup checkstyle issues

8014c6e

zendern marked this pull request as ready for review March 7, 2022 02:03

yawkat previously approved these changes Mar 7, 2022

View reviewed changes

sdelamo requested changes Mar 7, 2022

View reviewed changes

zendern added 4 commits March 7, 2022 13:23

Better javadoc

84db7da

Using constants

16b05b6

Groovy-ify and use types more

075cb6c

Always return an array

e960455

zendern dismissed yawkat’s stale review via e960455 March 7, 2022 23:16

Add @nonnull

67394be

sdelamo reviewed Mar 8, 2022

View reviewed changes

yawkat previously approved these changes Mar 9, 2022

View reviewed changes

yawkat added 4 commits March 14, 2022 10:22

stop using the old CertificateEvent constructor to fix warning

3b9718d

checked cast from generateCertificates

688ccb2

add a check that the certificates in CertificateEvent aren't empty

ff5e044

handle empty cert array

a33236c

yawkat dismissed their stale review via a33236c March 14, 2022 09:46

yawkat requested a review from sdelamo March 14, 2022 09:47

yawkat previously approved these changes Mar 14, 2022

View reviewed changes

Change AcmeService to return Optional

6a5dd44

sdelamo dismissed yawkat’s stale review via 6a5dd44 March 16, 2022 17:30

sdelamo approved these changes Mar 16, 2022

View reviewed changes

sdelamo merged commit 2b4eadd into micronaut-projects:master Mar 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using full cert chain over single certificate #161

Using full cert chain over single certificate #161

zendern commented Jul 19, 2021

zendern Jul 19, 2021

michaelkoetter Jul 19, 2021

zendern Jul 19, 2021

yawkat commented Mar 2, 2022

zendern commented Mar 7, 2022 •

edited

Loading

sdelamo left a comment

sdelamo Mar 8, 2022 •

edited

Loading

sdelamo Mar 16, 2022

sdelamo Mar 8, 2022 •

edited

Loading

sdelamo Mar 16, 2022

recursivecodes commented Mar 11, 2022

yawkat commented Mar 14, 2022

recursivecodes commented Mar 16, 2022

yawkat commented Mar 16, 2022

sdelamo commented Mar 16, 2022

recursivecodes commented Mar 17, 2022

recursivecodes commented Mar 17, 2022

		@@ -24,6 +24,7 @@
		public class CertificateEvent {
		private final X509Certificate certificate;

Using full cert chain over single certificate #161

Using full cert chain over single certificate #161

Conversation

zendern commented Jul 19, 2021

zendern Jul 19, 2021

Choose a reason for hiding this comment

michaelkoetter Jul 19, 2021

Choose a reason for hiding this comment

zendern Jul 19, 2021

Choose a reason for hiding this comment

yawkat commented Mar 2, 2022

zendern commented Mar 7, 2022 • edited Loading

sdelamo left a comment

Choose a reason for hiding this comment

sdelamo Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

sdelamo Mar 16, 2022

Choose a reason for hiding this comment

sdelamo Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

sdelamo Mar 16, 2022

Choose a reason for hiding this comment

recursivecodes commented Mar 11, 2022

yawkat commented Mar 14, 2022

recursivecodes commented Mar 16, 2022

yawkat commented Mar 16, 2022

sdelamo commented Mar 16, 2022

recursivecodes commented Mar 17, 2022

recursivecodes commented Mar 17, 2022

zendern commented Mar 7, 2022 •

edited

Loading

sdelamo Mar 8, 2022 •

edited

Loading

sdelamo Mar 8, 2022 •

edited

Loading