Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

acme4j 2.0 on maven central incompatible with recent pebble #59

Closed
milux opened this issue Mar 14, 2018 · 6 comments
Closed

acme4j 2.0 on maven central incompatible with recent pebble #59

milux opened this issue Mar 14, 2018 · 6 comments

Comments

@milux
Copy link

milux commented Mar 14, 2018

It seems that the code published as stable 2.0 acme4j release on maven central contains code that is incompatible with the recent version of pebble, i.e. somewhat deprecated.
This also happened to me yesterday with my old 2.0-SNAPSHOT build, before the start of the official new ACMEv2 API at letsencrypt. However, updating to the most recent 2.0-SNAPSHOT based on your master branch fixed this issue.
Recent 2.0-SNAPSHOT builds do not suffer from this problem.
This is the stack trace when using the most recent pebble (based on master branch from letsencrypt repo) together with acme4j 2.0 from maven central:

Exception in thread "main" java.lang.RuntimeException: org.shredzone.acme4j.exception.AcmeServerException: Challenge response body contained legacy KeyAuthorzation field, POST body should be `{}`
	at de.fhg.aisec.ids.acmeclient.AcmeClientService.lambda$requestCertificate$2(AcmeClientService.java:105)
	at java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:184)
	at java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:193)
	at java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1382)
	at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:481)
	at java.util.stream.ForEachOps$ForEachTask.compute(ForEachOps.java:291)
	at java.util.concurrent.CountedCompleter.exec(CountedCompleter.java:731)
	at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289)
	at java.util.concurrent.ForkJoinTask.doInvoke(ForkJoinTask.java:401)
	at java.util.concurrent.ForkJoinTask.invoke(ForkJoinTask.java:734)
	at java.util.stream.ForEachOps$ForEachOp.evaluateParallel(ForEachOps.java:160)
	at java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateParallel(ForEachOps.java:174)
	at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:233)
	at java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:418)
	at de.fhg.aisec.ids.acmeclient.AcmeClientService.requestCertificate(AcmeClientService.java:87)
	at de.fhg.aisec.ids.acmeclient.AcmeClientService.main(AcmeClientService.java:35)
Caused by: org.shredzone.acme4j.exception.AcmeServerException: Challenge response body contained legacy KeyAuthorzation field, POST body should be `{}`
	at org.shredzone.acme4j.connector.DefaultConnection.throwAcmeException(DefaultConnection.java:441)
	at org.shredzone.acme4j.connector.DefaultConnection.performRequest(DefaultConnection.java:366)
	at org.shredzone.acme4j.connector.DefaultConnection.sendSignedRequest(DefaultConnection.java:178)
	at org.shredzone.acme4j.connector.DefaultConnection.sendSignedRequest(DefaultConnection.java:157)
	at org.shredzone.acme4j.challenge.Challenge.trigger(Challenge.java:146)
	at de.fhg.aisec.ids.acmeclient.AcmeClientService.lambda$requestCertificate$2(AcmeClientService.java:91)
	... 15 more
@milux
Copy link
Author

milux commented Mar 14, 2018

BTW: Thank you for this great library! I tend to speak about issues too quickly without honoring the good job people like you do. 👍

@shred
Copy link
Owner

shred commented Mar 14, 2018

Yes... This is related to letsencrypt/pebble@65f06474. Unfortunately, Let's Encrypt staging and production servers do not incorporate this change yet. I had the choice to maintain Pebble support and break staging/production support, or vice versa. I decided to break Pebble in acme4j 2.0, for obvious reasons.

acme4j 2.0 was built from the staging branch that does not contain the Pebble change. The master branch contains this change. As soon as Let's Encrypt staging and production servers incorporate that change too, I will merge the staging branch back to master. So acme4j 2.1 should work with both Pebble and the public servers again.

For the time being, if you need Pebble support, you'll have to build from master.

I'm not happy with that solution myself, but I saw no other way to resolve this. I'm closing this bug because there is no other solution than to wait for Let's Encrypt.

Thank you for your nice words! I really appreciate it. 😄

@shred shred closed this as completed Mar 14, 2018
@cpu
Copy link
Contributor

cpu commented Mar 14, 2018

Yes... This is related to letsencrypt/pebble@65f0647. Unfortunately, Let's Encrypt staging and production servers do not incorporate this change yet. I had the choice to maintain Pebble support and break staging/production support, or vice versa. I decided to break Pebble in acme4j 2.0, for obvious reasons.

My intention with making Pebble more aggressive than staging/prod ACME v2 was to help provide a way for ACME clients to easily test if the upcoming changes will break their client. In retrospect I created a situation where you can either support Pebble, or the prod/staging environment. Not ideal! I'll probably revert the Pebble change today and will think about this more carefully in the future. ACME4J wasn't the only client bit by this.

Thanks for your patience/feedback folks.

@milux
Copy link
Author

milux commented Mar 14, 2018

@shred Thank you, perfectly understandable!

@cpu I can somewhat understand why you did this, but as you said, this is "not ideal" indeed...
I think pebble should absolutely enforce the latest API version, but not before boulder also supports it.
Otherwise, pebble is not suitable for real-world testing, which makes it basically useless for around 99% of its current users like me, who want to ensure that their implementation reflects state-of-the-art practices.

@cpu
Copy link
Contributor

cpu commented Mar 14, 2018

To keep everything self-contained, this issue is now fixed in Pebble with -strict false (the current default).

@shred
Copy link
Owner

shred commented Mar 14, 2018

Thank you, @cpu.

I have reverted commit 1987d95 and merged the staging branch into master. master is now essentially in the state of acme4j 2.0, as it should be. staging is not needed any more, and was deleted.

I will revert the revert 😉 when staging and production is able to handle challenges without authorizations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants