Adding list leaf for SSH public keys#1498
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an authorized-ssh-keys list to the OpenConfig AAA model to allow users to have multiple SSH public keys. Feedback indicates that the current placement of the list within a grouping violates OpenConfig structural standards, where lists should be siblings to config and state containers rather than nested within them. Additionally, the max-elements 10 constraint is viewed as overly restrictive, and the description for the authorized-ssh-key leaf should be updated to specify supported key formats like RSA or DSA.
| leaf ssh-key { | ||
| type string; | ||
| description | ||
| "SSH public key for the user (RSA or DSA)"; | ||
| } |
There was a problem hiding this comment.
Should this single ssh-key leaf be deprecated?
There was a problem hiding this comment.
Would that break backwards compatibility?
There was a problem hiding this comment.
No - not until fully removed. If introducing an alternate method as is in this patch set then the original leaf should be marked as status deprecated along w/ a description update to transition to this new method
earies
left a comment
earies
left a comment
There was a problem hiding this comment.
Overall in support of this change as it aligns w/ most implementations tmk - Just a few comments surrounding how the key/data should be decomposed and indexed.
| description | ||
| "Configuration data for user SSH keys"; | ||
|
|
||
| leaf key-name { |
There was a problem hiding this comment.
| leaf key-name { | |
| leaf name { |
We already know these are keys and I think the normal index here for uniqueness would correspond to the "comment" of a public key
| leaf authorized-ssh-key { | ||
| type string; | ||
| description | ||
| "Authorized SSH public key for the user (RSA or ECDSA)"; |
There was a problem hiding this comment.
Not sure I see a reason to limit the key type in the comment but this does weigh the question if the type should be it's own separate leaf essentially decomposing the distinct fields of a public key representation - if so this could likely be an identityref to well known algorithms. This field that holds the key data could be specified that this should be B64 encoded
3 participants
Change Scope
Platform Implementations
https://github.com/openconfig/gnsi/tree/main/credentialz#update-the-clients-authorized-key
The platform supports adding more than one public key per user
Tree View
module: openconfig-aaa +--rw aaa | +--rw config | +--ro state | +--rw authentication | | +--rw glome | | | +--ro state | | +--rw config | | | +--rw authentication-method* union | | +--ro state | | | +--ro authentication-method* union | | +--rw admin-user | | | +--rw config | | | | +--rw admin-password? string | | | | +--rw admin-password-hashed? oc-aaa-types:crypt-password-type | | | +--ro state | | | +--ro admin-password? string | | | +--ro admin-password-hashed? oc-aaa-types:crypt-password-type | | | +--ro admin-username? string | | +--rw users | | +--rw user* [username] | | +--rw username -> ../config/username | | +--rw config | | | +--rw username? string | | | +--rw password? string | | | +--rw password-hashed? oc-aaa-types:crypt-password-type | | | +--rw ssh-key? string | | | +--rw role union | | +--ro state | | +--ro username? string | | +--ro password? string | | +--ro password-hashed? oc-aaa-types:crypt-password-type | | +--ro ssh-key? string | | +--ro role union + | | +---- authorized-ssh-keys + | | +---- authorized-ssh-key* [key-name] + | | +---- key-name? -> ../config/key-name + | | +---- config + | | | +---- key-name? string + | | | +---- authorized-ssh-key? string + | | +--ro state + | | +--ro key-name? string + | | +--ro authorized-ssh-key? string