introduce send_device_attestation() for device-attest-01 challenge #126
Conversation
|
If the intent is to support device-attest-01, wouldn't we also need the challenge & identifier types? Maybe some broader motivation could be put into the PR desc :-) More generally I'm wondering if it's premature to implement support here. AFAICT the cited ID hasn't been adopted by the ACME WG yet and is pretty early days. We added profiles support in a similar state so there's some precedent for that being OK, but in that case the diff is pretty small, and there was also both a convenient test server implementation (Pebble) and production deployment (LE). I could be convinced it's stable enough to merit inclusion in the instant-acme API but I think would prefer to land all the parts at once vs piece by piece. WDYT? |
Given that the linked URL is /doc/draft-acme-device-attest/ I think the Generally agree it's probably early but on the other hand I like being somewhat early to support things that look serious enough. Also not too afraid of revving the API semver-incompatibly if necessary especially if breakage is confined to the specific use case. |
Oops, you're 100% right. The draft page says:
That's helpful 👍 I think if all the pieces were put together into one PR (separate commits are OK ofc) then I'm onboard. |
christianhoelzl
changed the title
christianhoelzl
force-pushed
the
set_challenge_with_payload
branch
from
d6ffd8c to
0499228
Compare
|
This is an interesting discussion. I also feel that it still is in an early stage. On the other hand, there already is support for this ACME challenge device-attest-01 in Step CA and maybe others. Regarding the API, if limiting the new challenge set ready function to device attestation, then the If you look into the acme-device-attest demo from Brandon Weeks, the ACME server is Step CA and the ACME client based on a GO library from Google. This library provids an arbitrary payload for newer challenges, look here. So I think long term the API could end up having both, challenge-specific and a generic function to add an arbitrary payload for other challenges beyond device-attest-01. |
christianhoelzl
force-pushed
the
set_challenge_with_payload
branch
from
0499228 to
3c306f3
Compare
|
Rebased onto main |
christianhoelzl
force-pushed
the
set_challenge_with_payload
branch
from
3c306f3 to
0acd83f
Compare
|
Fixed both review comments (DeviceAttest before Empty, and Cow). |
cpu
left a comment
cpu
left a comment
There was a problem hiding this comment.
What about the two new Identifier types?
AIUI these can be included in a CSR. It would be fine to ignore them for the rcgen CSR generation code that this crate offers as a helper, but I think in all cases the CSR's identifiers need to match the order's identifiers, so an order would have to be constructed with the new identifier types even if you gin up a CSR externally, right?
christianhoelzl
force-pushed
the
set_challenge_with_payload
branch
from
0acd83f to
993883d
Compare
|
I rebased and fixed the review comments (two new identifiers, better description). Thanks for the suggestions. |
cpu
left a comment
cpu
left a comment
There was a problem hiding this comment.
Thanks!
One last thought: have you tested this end-to-end with a server implementation? e.g. maybe that fork of step-ca you found? I think it'd be nice to confirm this works in practice before we merge it. I'm still a little bit nervous there's no test coverage.
| /// Permanent Identifier | ||
| /// | ||
| /// Note that this identifier is only used for attestation. | ||
| PermanentIdentifier(String), | ||
|
|
||
| /// Hardware Module identifier | ||
| /// | ||
| /// Note that this identifier is only used for attestation. | ||
| HardwareModule(String), |
There was a problem hiding this comment.
I went down a bit of a rabbit hole thinking about what the value of these identifier types should be. I think String is the right choice for now, but I also filed some issues upstream about the spec because I think as written it leaves some important details up in the air w.r.t how CSRs and order identifiers interact:
brandonweeks/draft-bweeks-acme-device-attest#9
brandonweeks/draft-bweeks-acme-device-attest#10
There was a problem hiding this comment.
Thats great! If it turns out, that there is something better than String then I'll provide a pr.
|
I have partially tested with a private ACME test server such that the new method works fine. Testing with real hw and e.g. step-ca is on my todo list. Device-attestation is well documented for recent releases of step-ca (Brandon Weeks contributed to step-ca on this topic). |
I would be OK merging on the basis of this testing if @djc concurs |
3 participants
This commit adds a new function
Order::send_device_attestation(). This function can be used for a device-attest-01 challenge, that requires a payload to be provided when the ACME client notifies the ACME server that the challenge is ready. The challenge is described in the draft-acme-device-attest. Pebble has no support for this, so it is untested so far in the pebble test.This new API method allows to use the ACME challenge device-attest-01. Motivation to use this challenge is to improve device management based on the methods described in the link.