Skip to content

Add credentialz endpoints to manage system role accounts. #207

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.

Sign up for GitHub

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

Open
avanpo wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add credentialz endpoints to manage system role accounts. #207

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions credentialz/credentialz.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,16 @@ service Credentialz {
// Get the current public keys from the host. Each configured key will be
// returned in the provided list.
rpc GetPublicKeys(GetPublicKeysRequest) returns (GetPublicKeysResponse);

// Add a system role account.
Comment thread
morrowc marked this conversation as resolved.
Copy link
Copy Markdown

@LimeHat LimeHat Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why my previous comment was marked as resolved, but I still would like to get an answer to that question.

Would be good to add a strict definition what a "system role account" is.

OpenConfig has SYSTEM_ROLE_ADMIN https://github.com/openconfig/public/blob/db78d44e93a97cef4db27f82bba3fba454e907a5/release/models/system/openconfig-aaa-types.yang#L77-L84, but I have a feeling that you mean something else in this context.

Copy link
Copy Markdown
Contributor

@morrowc morrowc Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 'system role admin' is like 'admin group' or 'root access for user in this group', right?
I think the intent of this is to provide a way to set a passwd/ssh-key/etc for:
linuxadmin
root
admin
event-manager-user

etc.. accounts which the vendor provides on the device outside of any requirement of the operator of that device.

Copy link
Copy Markdown

@LimeHat LimeHat Aug 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the intent of this is to provide a way to set a passwd/ssh-key/etc for:

is it? the proto change says it can only add or delete account. for credentials management we have existing credz service RPCs..

Copy link
Copy Markdown
Contributor

@morrowc morrowc Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect that 'system role account' here might have meant (and I'm sure i mis-read this originally):
"I want to add a user to the config"
or:
"I want to delete a user from the config"

Today credentialz can't really add or remove accounts. This change should permit us to get to that state, right?

// Note that an attempt to add a system role account that already exists will
// be rejected with an error.
rpc AddAccount(AddAccountRequest) returns (AddAccountResponse);

// Delete a system role account.
// Note that an attempt to delete a system role account that doesn't exist
// will be rejected with an error.
rpc DeleteAccount(DeleteAccountRequest) returns (DeleteAccountResponse);
}

//
Expand Down Expand Up @@ -787,3 +797,17 @@ message GetPublicKeysResponse {
// Return all currently configured keys on host.
repeated PublicKey public_keys = 1;
}

message AddAccountRequest {
// The system role account name. This account must not already exist.
string account = 1;
Copy link
Copy Markdown
Contributor

@nmahabaleshwar nmahabaleshwar Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some implementations need a password to create an account, would suggest add Password which can be optional.

We might also want to clarify if Password is not specified we want to create an account with,

  1. Disabled password (prevent authentication via password) OR
  2. No password (password not required for authentication)

Copy link
Copy Markdown
Contributor

@morrowc morrowc Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(picking long past old PR)

Either specifying in the proto/docs:
"If a passwd is required, but not provided, an error is returned"
or:
"where a password is required and not provided the device will provide an unusable password"
(like username:*:userid:groupid:comment:shell on standard unix-like platforms)

seems ok.

}

message AddAccountResponse {}

message DeleteAccountRequest {
// The system role account name. This account must exist.
string account = 1;
}

message DeleteAccountResponse {}