fix: send_device_attestation yields challenge status#254
Conversation
35c7e9b to
2de7aaf
Compare
djc
left a comment
There was a problem hiding this comment.
LGTM -- is this from a spec change, or just a different kind of testing?
|
This is from a different kind of testing with a real CA (Step CA), not from a spec change. |
|
Okay. I think it would be good to add some testing for this stuff. In an ideal world, it would be nice to get support for this draft into Pebble -- do you think that's something that could be in scope for you? |
|
Tough question. That could be in scope. I thought about automated testing for a while now and there a some options:
|
|
Forking Pebble doesn't seem appealing, I don't think adding a custom Rust-based ACME server would help if we build it. Would be good if you can file an issue against Pebble to at least ask if they'd be open to supporting it (and linking that here). |
cpu
left a comment
There was a problem hiding this comment.
As mentioned in https://github.com/djc/instant-acme/pull/255/changes#r2799709957 I think this is semver breaking, but I don't think it's worth bumping our minor version or leaving it broken. I suspect adoption is very low and provided we mark this stuff experimental it feels like the right-trade off to ignore semver and fix it.
2de7aaf to
1a60792
Compare
|
To mark the device-attest as experimental, I could add a bullet "* Device attestation support is experimental" to the section Liminations in the README.md. Or better add this sentence to the struct |
|
I like the idea of doing it both in README and in the related rustdoc of the types/functions. |
cpu
left a comment
There was a problem hiding this comment.
Thanks, LGTM mod the rust fmt CI failure.
16e0c7e to
af8bec4
Compare
The pull request extends the function
send_device_attestation. It yields theChallengeStatusinstead of an empty success result. This is useful to check if the attestation object was validated by the ACME CA immediately after sending the attestation object.