-
Notifications
You must be signed in to change notification settings - Fork 43
Async enrollment client implementation #11889
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
base: master
Are you sure you want to change the base?
Conversation
cbcf4ae to
9b8670b
Compare
c54e031 to
83a885a
Compare
9b8670b to
f368186
Compare
83a885a to
860068f
Compare
f368186 to
ef0b1d1
Compare
860068f to
3c706b7
Compare
ef0b1d1 to
18b27cb
Compare
1278012 to
a58c868
Compare
18b27cb to
e8b6106
Compare
a58c868 to
4549912
Compare
e8b6106 to
e7e3f87
Compare
4549912 to
cdaa7c1
Compare
e7e3f87 to
cef2c16
Compare
cdaa7c1 to
1140e87
Compare
cef2c16 to
094cb42
Compare
1140e87 to
746fbe7
Compare
094cb42 to
3ace11f
Compare
746fbe7 to
1fb8f2e
Compare
3ace11f to
6ff3a33
Compare
1fb8f2e to
bb9ca8f
Compare
01f12ca to
5a53419
Compare
bb9ca8f to
ad4207a
Compare
bdd6080 to
0b8b208
Compare
AureliaDolo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few places with todos left, will they b tackled in this PR or later ?
| SubmitPayloadSignature::PKI { .. } => { | ||
| // TODO: use `libparsec_platform_pki` to obtain info on | ||
| // the X509 root certificate from the submitter X509 certificate | ||
| todo!() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
link issue ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
| // PKI-related errors | ||
| #[error("Invalid X509 trustchain (server doesn't recognize the root certificate)")] | ||
| InvalidX509Trustchain, | ||
| // TODO: add other PKI-related errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
| // PKI-related errors | ||
| #[error("Invalid X509 trustchain (server doesn't recognize the root certificate)")] | ||
| InvalidX509Trustchain, | ||
| // TODO: add other PKI-related errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
| // PKI-related errors | ||
| #[error("Invalid X509 trustchain (server doesn't recognize the root certificate)")] | ||
| InvalidX509Trustchain, | ||
| // TODO: add other PKI-related errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
|
|
||
| pub type SubmitterForgetAsyncEnrollmentError = libparsec_platform_device_loader::RemoveDeviceError; | ||
|
|
||
| pub async fn submitter_forget_async_enrollment( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a note in the RFC about the forget operation, even if it's only local.
(maybe it would be worth adding a forget server cmd to clean it up as soon as we may know that an enrollment is no longer usable, (maybe not caring about the success of the remote part of the operation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a note in the RFC about the forget operation, even if it's only local.
done 👍
maybe it would be worth adding a forget server cmd to clean it up as soon as we may know that an enrollment is no longer usable, (maybe not caring about the success of the remote part of the operation
I'd rather not do that since it increases complexity (more APIs, more corner cases given we have to deal with offline errors) and cover only a niche case.
Instead let just wait for end-user feedback before adding more feature to the async enrollment.
| libparsec_client_connection::ProxyConfig::default(), | ||
| )?; | ||
|
|
||
| // TODO: Add `openbao_transit_mount_path` to `DeviceAccessStrategy::OpenBao` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is implemented in the next PR #11963
| openbao_auth_token, | ||
| } => { | ||
| let client = libparsec_client_connection::build_client()?; | ||
| // TODO: Add `openbao_transit_mount_path` to `DeviceAccessStrategy::OpenBao` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is implemented in the next PR #11963
0b8b208 to
d6a46dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move the changes in platform_device_loader to another pr ? It would be easier for #11954 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least the kind that are changing existing functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#12022 😿
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note I've moved platform_device_loader's changes unrelated to openbao in there own commit
c4f670b to
4dfcd83
Compare
03113be to
eb4ee54
Compare
docs/rfcs/1023-async-enrollment.md
Outdated
| > So instead Alice is only expected to remove the enrollment info from her machine's filesystem | ||
| > (hence there is no longer an enrollment pending from her point of view), and an admin | ||
| > should manually cancel the enrollment (but accepting it by mistake is not an issue either, | ||
| > since Alice no longer knows about here user & device secret keys). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| > since Alice no longer knows about here user & device secret keys). | |
| > since Alice no longer knows about her user & device secret keys). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 done.
| > (as we consider this rather unusual). | ||
| > So instead Alice is only expected to remove the enrollment info from her machine's filesystem | ||
| > (hence there is no longer an enrollment pending from her point of view), and an admin | ||
| > should manually cancel the enrollment (but accepting it by mistake is not an issue either, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't that create a user for Alice and then if Alice eventually wants to join the organization there will be a phantom Alice to revoke first ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes exactly, I've added a word about this 👍
eb4ee54 to
98fe18a
Compare
No description provided.