Skip to content
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

credentialz: clarify the expected format of authorized_key field #210

Closed
LimeHat opened this issue Nov 22, 2024 · 4 comments
Closed

credentialz: clarify the expected format of authorized_key field #210

LimeHat opened this issue Nov 22, 2024 · 4 comments

Comments

@LimeHat
Copy link

LimeHat commented Nov 22, 2024

Currently AuthorizedKey message does not define the exact format of the key that is expected to be present in the message.

message AccountCredentials {
message AuthorizedKey {
bytes authorized_key = 1;
// Options specified for this authorized key.
repeated Option options = 2;
// Encryption mode for entry.
KeyType key_type = 3;
// An optional description of the key.
string description = 4;
}

Even if we can assume that openssh rfc4253-based keys are used, then three interpretations are possible:

  1. authorized_key should include a full pubkey string in the form of type key comment
  2. authorized_key should include only the key bytes, making a non-zero value of the KeyType enum mandatory
  3. both of the above should be allowed

This should be clarified in the .proto description.

cc @marcushines @morrowc @robshakir

@morrowc
Copy link
Contributor

morrowc commented Nov 24, 2024

In some other places we specified the key type with an enum (I thought) is there a reason to not just add the fields and proper enums here?

@melzhan
Copy link

melzhan commented Nov 25, 2024

authorized_key should include a full pubkey string in the form of type key comment

AuthorizedKey has fields authorized_key key_type and description. Would it not be natural to assume that those correspond to:
authorized_key = key bytes
key_type = type of key
description = comment

Unless the key_type and description fields are meant for something else...?

@morrowc
Copy link
Contributor

morrowc commented Nov 26, 2024

mel, that does seem like the correct mapping to me...

Perhaps we should (maybe this was @LimeHat 's point) just update the docs in the proto to make this clear?

how's: #211

@LimeHat
Copy link
Author

LimeHat commented Nov 26, 2024

Would it not be natural to assume that those correspond to:
authorized_key = key bytes

Not necessarily.
"authorized key" is an ambiguous term. For some it is natural to assume unix authorized_keys format which implies the full string (e.g. ssh-rsa <public_key_bytes>).
Other proto fields can be omitted (unless something says that they cannot; but nothing says that in the proto).

In some other places we specified the key type with an enum (I thought) is there a reason to not just add the fields and proper enums here?

As long as there's a strict definition of the expected inputs, I'm fine with it; the main issue is the ambiguity.

I think for a typical user (outside of google) the direct consumption of a ssh-keygen output is a simpler approach (no need to do any parsing magic), but it comes down to preferences.

Perhaps we should (maybe this was @LimeHat 's point) just update the docs in the proto to make this clear?

Yes, choose which approach you think is correct (there are at least three possible options I indicated above, and that is only under the assumption that we're sticking to the single format) and explicitly document it.

@LimeHat LimeHat closed this as completed Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants