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

Jump to bottom

x/crypto/acme/autocert: reason acme challenge fails to complete is not logged #19800

Closed
groob opened this issue Mar 31, 2017 · 11 comments
Closed

x/crypto/acme/autocert: reason acme challenge fails to complete is not logged #19800

groob opened this issue Mar 31, 2017 · 11 comments
Labels
FrozenDueToAge
Milestone
Unreleased

Comments

@groob
Copy link
Contributor

groob commented Mar 31, 2017 •
edited

If a Let's Encrypt challenge fails with autocert, it's very difficult to debug the actual reason the challenge was invalid.

Logs show that authorization failed, but not the reason. Additionally, due to #17740 unless the server is restarted, the identifier authorization failed log line only shows up once.

2017/03/29 13:47:35 http: TLS handshake error from x.x.x.x:49267: acme: identifier authorization failed
2017/03/29 13:47:35 http: TLS handshake error from x.x.x.x:49268: acme/autocert: missing certificate

A simple solution, if the authorization failed would be to log an additional line with the let's encrypt authorization URL here:
https://github.com/golang/crypto/blob/3cb07270c9455e8ad27956a70891c962d121a228/acme/autocert/autocert.go#L509

Another option would be to create and log a custom error type, like RemoteError in xenolf/xego
Changing the error type would be a breaking change though.
@odeke-em
Copy link
Member

/cc @x1ddos

@x1ddos
Copy link

x1ddos commented Mar 31, 2017 •
edited

Well, we already have the Error type but it isn't used in that case. Instead, ErrAuthorizationFailed is returned. It makes sense in the context of the acme client, but yeah, autocert then throws that information away.

I suppose we could enrich the error returned by GetCertificate but I'm a bit worried about this bit:

The error is propagated back to the caller of GetCertificate and is user-visible.

Not sure about logging. The autocert package has never logged, it doesn't even import log pkg.

Other ideas?

@x1ddos
Copy link

x1ddos commented Mar 31, 2017

Maybe we could use something like expvar or a similar way to expose stats and errors of autocert.

@groob
Copy link
Contributor Author

groob commented Mar 31, 2017 •
edited

Not sure about logging. The autocert package has never logged, it doesn't even import log pkg.

Ah I agree, I mentioned it because net/http logs, but I'd be against importing log here.
I would like to log the error in my own application.

IMO, the best approach would be either returning the ACME error type, or annotating the sentinel error with fmt.Errorf.

@x1ddos
Copy link

x1ddos commented Mar 31, 2017

@bradfitz what do you think?

@gopherbot gopherbot added this to the Unreleased milestone Mar 31, 2017
@bradfitz
Copy link
Contributor

bradfitz commented Apr 2, 2017

@x1ddos, we make no API promises in this package, and I also don't think it would affect many people, so I think it's okay to delete var ErrAuthorizationFailed and replace it with type AuthorizationError struct with more rich info. Let's do that first, and then discuss what to do with that richer info in the autocert package.

@gopherbot
Copy link

CL https://golang.org/cl/40450 mentions this issue.

gopherbot pushed a commit to golang/crypto that referenced this issue Apr 25, 2017
acme: replace ErrAuthorizationFailed with a type
141c762 
This provides acme users with more insights into authorization failures.

Updates golang/go#19800.

Change-Id: I821298a6c8bd21fc517b2ab9128dd3d32be90249
Reviewed-on: https://go-review.googlesource.com/40450
Run-TryBot: Alex Vaghin <ddos@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@x1ddos x1ddos mentioned this issue Jul 31, 2017
Open
@munnerz
Copy link

munnerz commented Oct 26, 2017

Can anyone advise when CL 40450 (mentioned above) will make it onto golang.org/x/crypto/acme? This is making the UX of our cert-manager project pretty poor when authorisations fail.

Alternatively, is there some way I can use an experimental version of the package in my project?

@munnerz
Copy link

munnerz commented Oct 26, 2017

Ignore me, I have been stupid and just realised that the repo is already up to date, and the error messages are still just not complete (don't include the full body if you dump the raw message).

I will update this issue/create a new issue for this, and perhaps a CL too.

@x1ddos
Copy link

x1ddos commented Nov 3, 2017

@munnerz what kind of errors do you get? It's supposed to be in Error.Detail field, at least for a well-behaved ACME CA.

Also, check Error.ProblemType of the same struct. It's good for programmatic decisions about errors.

@gopherbot
Copy link

Change https://golang.org/cl/115938 mentions this issue: acme/autocert: surface details of acme.AuthorizationError

@golang golang locked and limited conversation to collaborators Jun 14, 2019
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 28, 2022
@FiloSottile
acme/autocert: surface details of acme.AuthorizationError
aac3f65 
Fixes golang/go#19800

Change-Id: If915a70f4dee78e71dcfc487726cdf83d45b4d50
Reviewed-on: https://go-review.googlesource.com/115938
Reviewed-by: Alex Vaghin <ddos@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
c-expert-zigbee added a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
@c-expert-zigbee
acme/autocert: surface details of acme.AuthorizationError
60db39d 
Fixes golang/go#19800

Change-Id: If915a70f4dee78e71dcfc487726cdf83d45b4d50
Reviewed-on: https://go-review.googlesource.com/115938
Reviewed-by: Alex Vaghin <ddos@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
c-expert-zigbee added a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
@c-expert-zigbee
acme/autocert: surface details of acme.AuthorizationError
2accda4 
Fixes golang/go#19800

Change-Id: If915a70f4dee78e71dcfc487726cdf83d45b4d50
Reviewed-on: https://go-review.googlesource.com/115938
Reviewed-by: Alex Vaghin <ddos@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
LewiGoddard pushed a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
@FiloSottile
acme/autocert: surface details of acme.AuthorizationError
7cc6dca 
Fixes golang/go#19800

Change-Id: If915a70f4dee78e71dcfc487726cdf83d45b4d50
Reviewed-on: https://go-review.googlesource.com/115938
Reviewed-by: Alex Vaghin <ddos@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
BiiChris pushed a commit to BiiChris/crypto that referenced this issue Sep 15, 2023
@FiloSottile
acme/autocert: surface details of acme.AuthorizationError
67152cf 
Fixes golang/go#19800

Change-Id: If915a70f4dee78e71dcfc487726cdf83d45b4d50
Reviewed-on: https://go-review.googlesource.com/115938
Reviewed-by: Alex Vaghin <ddos@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
6 participants
@bradfitz @x1ddos @munnerz @groob @odeke-em @gopherbot