GoogleCloudPlatform / guest-oslogin

OS Login Guest Environment for Google Compute Engine
https://cloud.google.com/compute/docs/oslogin/
Apache License 2.0
95 stars 47 forks source link

sshca: support method token and handle multi line #109

Closed dorileo closed 1 year ago

dorileo commented 1 year ago

We didn't account that the SSH_AUTH_INFO_0 variable format has a method token and that it can have multiple lines - even that it's unlikely to happen with oslogin use cases it's healthier to account for that.

The tests were changed to reflect the actual implementation change.

Additionally this patch also changes the internal function's signature to be prefixed with _.

google-oss-prow[bot] commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dorileo

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: - ~~[OWNERS](https://github.com/GoogleCloudPlatform/guest-oslogin/blob/master/OWNERS)~~ [dorileo] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
dorileo commented 1 year ago

/hold

ericdand commented 1 year ago

There appear to be three separate changes jammed together in this CL:

  1. The actual change named in the description.
  2. Renaming a large number of private functions to have a leading _.
  3. Adjusting a bunch of test data to include the string "publickey".

At the very bare minimum, the description should state that these changes are being made. However, I would greatly prefer that you at least split the feature change of 1 apart from the cleanup changes of 2 and 3; ideally all three would be done in separate pull requests.

dorileo commented 1 year ago

Hi @ericdand

There appear to be three separate changes jammed together in this CL:

  1. The actual change named in the description.
  2. Renaming a large number of private functions to have a leading _.
  3. Adjusting a bunch of test data to include the string "publickey".

At the very bare minimum, the description should state that these changes are being made. However, I would greatly prefer that you at least split the feature change of 1 apart from the cleanup changes of 2 and 3; ideally all three would be done in separate pull requests.

Items 1 and 3 are related, we are changing the implementation so it makes sense to also change the test cases to reflect it. (added a comment to it into the commit message).

Item 2 is really unrelated, let me know if you want me to send a separate PR doing so - however I also changed the commit message to reflect it.

ericdand commented 1 year ago

/lgtm

Just updating the description is OK this time -- this change is important so let's not get too bogged down by process for process's sake, and just get it checked in.

dorileo commented 1 year ago

/unhold