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

x/crypto/acme: Rejected TOS results in inability to register #18379

Open
taralx opened this issue Dec 20, 2016 · 7 comments
Open

x/crypto/acme: Rejected TOS results in inability to register #18379

taralx opened this issue Dec 20, 2016 · 7 comments
Milestone

Comments

@taralx
Copy link

@taralx taralx commented Dec 20, 2016

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

1.6

What operating system and processor architecture are you using (go env)?

amd64

What did you do?

Call acme.Register with a prompt that returns false, then again with a prompt that succeeds.

What did you expect to see?

Success

What did you see instead?

Register returns a conflict without calling UpdateReg.

@taralx

This comment has been minimized.

Copy link
Author

@taralx taralx commented Dec 20, 2016

This mostly affects autocert -- if you initially return false from prompt, then you have to kill the key even if you later return true.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Dec 21, 2016

This is approximately the lowest priority bug. Don't expect things to work when you don't agree to the TOS. Actually, LetsEncrypt only has the TOS agreement in the protocol because it's a CA/B forum requirement to have a contractual relationship with their "customers". While we have the hook as a formality to respect the LetsEncrypt necessary legal formalities, you should just always return true.

/cc @x1ddos

@bradfitz bradfitz added this to the Unplanned milestone Dec 21, 2016
@bradfitz bradfitz changed the title crypto/acme: Rejected TOS results in inability to register x/crypto/acme: Rejected TOS results in inability to register Dec 21, 2016
@x1ddos

This comment has been minimized.

Copy link

@x1ddos x1ddos commented Dec 21, 2016

I think this is actually WAI. The acme package follows the spec very closely. So, acme.Register literally means perform the "registration" with the CA.

So, calling Register method regardless of the accept TOS return value will always result in a "conflict" error. Try calling it twice with accept TOS always returning true - you'll get a "conflict" error the second time.

@taralx so what are you proposing, add a logic to Register and invoke UpdateReg under certain conditions and don't otherwise? I'm afraid that may lead to unexpected behavior and will certainly break some existing clients which rely on that same fact that Register returns "conflict" error if called with an existing key.

This could be a feature request at best but not really sure it's worth it. Feels like very specific case and may cause more harm than good. Unless I'm missing something.

@taralx

This comment has been minimized.

Copy link
Author

@taralx taralx commented Dec 21, 2016

I'm fine with switching this to autocert if it's WAI for acme, but it seems weird that you only get a free UpdateReg the first time you call Register, after which you get a conflict error and no UpdateReg.

@taralx

This comment has been minimized.

Copy link
Author

@taralx taralx commented Dec 26, 2016

Opened #18433 for the autocert side.

@taralx

This comment has been minimized.

Copy link
Author

@taralx taralx commented Jan 2, 2017

follows the spec very closely. So, acme.Register literally means perform the "registration" with the CA.

If that's the case, why does it call UpdateReg at all? This is the problem IMO -- the behavior w.r.t. UpdateReg is inconsistent.

@x1ddos

This comment has been minimized.

Copy link

@x1ddos x1ddos commented Jan 4, 2017

@taralx you're right. it should be consistent. I'll try to come up with a fix in the next days.

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

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.