extract the principal from certs when cloud_run enabled #164
Conversation
|
Hi @coolheavynest. Thanks for your PR. I'm waiting for a GoogleCloudPlatform member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
ericdand
left a comment
There was a problem hiding this comment.
Generally LGTM, just a few style suggestions and a request for a comment or two more. :)
| opts.fingerprint = fingerprint; | ||
| opts.fp_len = fp_len; | ||
|
|
||
| if (cloud_run) { |
There was a problem hiding this comment.
Call me a functional programming purist, but IMO it's usually a bad code smell to reassign a variable outside of a loop.
You've already got the principal variable as a separate variable. Rather than replace user_name blindly here, please check that it's as expected (in this case, you expect it to always be "root," right?) and then call AuthorizeUser on the principal (i.e. call AuthorizeUser(principal, opts, *user_response, cloud_run) instead.
There was a problem hiding this comment.
Make sense! I'd put a root check for now. We probably need to support non-root user later but that will happen later.
src/oslogin_sshca.cc
Outdated
| } | ||
|
|
||
| if (GetString(&tmp_prin, &tmp_prin_len, principal, NULL) < 0) { | ||
| SysLogErr("Failed to read princiapl."); |
There was a problem hiding this comment.
Typo spotted. ;)
But also, what is this function call doing? It appears to be dissecting the valid_principals field, but it could use a comment explaining why it's valid to call this function with only 3 of 4 parameters set (previously it was only called with all 4, or with 2 of 4).
There was a problem hiding this comment.
I could put something like the field valid_principal is a self described/sized buffer. Just like how the follwoing extension field work. But tbh I don't really know hwo this works behind the scene (guess the peek32 does the trick). One thing I am sure about is that without dissecting itself the tmp_prin won't contain the correct content.
There was a problem hiding this comment.
Thanks for adding those comments. You're indeed using GetString correctly here, it just needed a bit of explaining. :)
| opts.fingerprint = fingerprint; | ||
| opts.fp_len = fp_len; | ||
|
|
||
| if (cloud_run) { |
src/oslogin_sshca.cc
Outdated
| } | ||
|
|
||
| if (GetString(&tmp_prin, &tmp_prin_len, principal, NULL) < 0) { | ||
| SysLogErr("Failed to read princiapl."); |
There was a problem hiding this comment.
Thanks for adding those comments. You're indeed using GetString correctly here, it just needed a bit of explaining. :)
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: coolheavynest, ericdand The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
ee0251d
into
GoogleCloudPlatform:master
…latform#164) * extract the principal from certs when cloud_run enabled * add comments for cert extractaion and use the principal for AuthorizeUser call
Always extract the principal from ssh certs.
When --cloud_run enabled, use principal as username for authentication.
Tested with
make sshcatests