Skip to content

Add secret auth api request#324

Merged
timothyF95 merged 4 commits intomainfrom
add-secret-auth-request
Mar 25, 2026
Merged

Add secret auth api request#324
timothyF95 merged 4 commits intomainfrom
add-secret-auth-request

Conversation

@timothyF95
Copy link
Copy Markdown
Contributor

@timothyF95 timothyF95 commented Mar 25, 2026

cre secrets create|update supports --secrets-auth browser: org-scoped encryption, digest binding, then the platform authorization step for this milestone (no browser redirect / vault submit yet).

Owner-key behavior unchanged

Note: Browser auth remains disabled in production.

func vaultPermissionForMethod(method string) (string, error) {
switch method {
case vaulttypes.MethodSecretsCreate:
return "VAULT_PERMISSION_CREATE_SECRETS", nil
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

list and delete support to be added in follow up PR

"cre workflow delete": {},
"cre account link-key": {},
"cre account unlink-key": {},
"cre secrets create": {},
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Needed to move to the execution layer as browser flow should not require RPC

@timothyF95 timothyF95 marked this pull request as ready for review March 25, 2026 11:01
@timothyF95 timothyF95 requested a review from a team as a code owner March 25, 2026 11:01
secID := &vault.SecretIdentifier{
Key: item.ID,
Namespace: item.Namespace,
Owner: orgID,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this the only difference compared to web3 flow or is there more? Can we reduce the amount of c/p code between those two functions?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, I have consolidated the duplicate code. This is a good simplification.


addr := common.HexToAddress(ownerAddress) // canonical 20-byte address
var label [32]byte
copy(label[12:], addr.Bytes()) // left-pad with 12 zero bytes
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are we getting rid of this code? If the other side expects 32 byte prefix in the payload, then should we keep it backwards compatible?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

if err := gqlClient.Execute(ctx, gqlReq, &gqlResp); err != nil {
return fmt.Errorf("could not complete the authorization request")
}
if gqlResp.CreateVaultAuthorizationURL.URL == "" {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So, this is just to complete the auth flow and receive JWT? We are not sending jsonrpc2.Request payload to the Gateway yet? That kind of confuses me in this function, because it feels like we should focus on the auth flow first, and then build jsonrpc2.Request for the requested secrets operation if the auth flow was successful.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nvm, I see it now, you need this because we need to send the request digest and it has to be attached to JWT. But are we missing the owner address in the GQL request? This is important if someone already has a secret owned by this address and now wants to switch over to org ownership.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is correct.
I have not yet implemented this. I need to think further on it. For now I am treating them as two separate flows.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

nvm, it's a small implementation so I have added it.

@timothyF95 timothyF95 added this pull request to the merge queue Mar 25, 2026
Merged via the queue into main with commit 1b145e7 Mar 25, 2026
22 checks passed
@timothyF95 timothyF95 deleted the add-secret-auth-request branch March 25, 2026 14:09
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

Successfully merging this pull request may close these issues.

2 participants