Skip to content

command/ca/token: support custom "user" claim #1375

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

Merged
merged 1 commit into from
Mar 4, 2025

Conversation

fuhry
Copy link
Contributor

@fuhry fuhry commented Feb 25, 2025

Add the --set and --set-file flags to the step ca token command, allowing the user to set keys in the "user" claim in the resulting JWT.

Name of feature:

Custom user data in tokens

Pain or issue this feature alleviates:

Lack of ability to pass custom trusted data to a template without a pre-existing CSR.

Is there documentation on how to use this feature? If so, where?

Yes, in the CLI help for step ca token.

In what environments or workflows is this feature supported?

Online JWT token flow

In what environments or workflows is this feature explicitly NOT supported (if any)?

Offline token flow, because cautils.OfflineTokenFlow doesn't support tokenOpts. Enforced by flags validation.

@github-actions github-actions bot added the needs triage Waiting for discussion / prioritization by team label Feb 25, 2025
@hslatman hslatman requested a review from maraino February 25, 2025 18:33
Copy link
Collaborator

@maraino maraino left a comment

Choose a reason for hiding this comment

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

Hi @fuhry, this looks quite good, but I noticed this doesn't work for SSH tokens:

$ step ca token --set foo=bar --set loo=asd --set-file <(echo '{"pree":1, "foor":{"1": "222"}}' ) --ssh localhost  | step crypto jwt inspect --insecure
✔ Provisioner: mariano (JWK) [kid: nvgnR8wSzpUlrt_tC3mvrhwhBx9Y7T1WL_JjcFVWYBQ]
Please enter the password to decrypt the provisioner key:
{
  "header": {
    "alg": "ES256",
    "kid": "nvgnR8wSzpUlrt_tC3mvrhwhBx9Y7T1WL_JjcFVWYBQ",
    "typ": "JWT"
  },
  "payload": {
    "aud": "https://ca.smallstep.com:8443/1.0/ssh/sign",
    "exp": 1740512971,
    "iat": 1740512671,
    "iss": "mariano",
    "jti": "36148a58bc56c0383f86f3d60e4d9f1fc7193d96319e52d2323a4d6d02ffb908",
    "nbf": 1740512671,
    "sha": "36c6b2a81b492ec11f2c3fd576716d91bd944c0e77bf9693e229e841b91ada09",
    "step": {
      "ssh": {
        "certType": "user",
        "keyID": "localhost",
        "principals": [],
        "validAfter": "",
        "validBefore": ""
      }
    },
    "sub": "localhost"
  },
  "signature": "zc5VERa4l3Hyu4wNuP3YgWUu2vPCLtq2VwbXUF9EPCqZ7kxSAfnX098C_rI-aIwePlib8PYymKcfM1a-qkR6tQ"
}

And there we have the step claim, I wonder if we want to add the user claim inside that. @hslatman what do you think about this?

@fuhry
Copy link
Contributor Author

fuhry commented Feb 25, 2025

@maraino Thanks for looking this over!

I noticed this doesn't work for SSH tokens

Thanks for flagging this - I'll look into what it will take to get this working for SSH tokens. Are there any other token types I should plan to support?

And there we have the step claim, I wonder if we want to add the user claim inside that. @hslatman what do you think about this?

I initially considered this and decided to make a separate user claim, because:

  • If user fields are added directly under "step", there arises a namespacing conflict between step-added fields (I believe ssh is the only one, presently) and user fields, such that one may overwrite the other
  • If "user" is made a sub-key under "step", you end up with excessive object depth and resulting template complexity, e.g. {{if hasKey .Token, "step"}} {{if hasKey .Token.step "user"}} {{if hasKey .Token.step.user, "custom_key"}}. Golang text/template doesn't do lazy evaluation so if the user tries to combine these with and, the template won't run.

So I think it's best to keep step and user separated.

Will request re-review after I've added and tested SSH token support.

@fuhry fuhry force-pushed the cli-token-set-option branch from 09c4b13 to 2da3a3f Compare February 25, 2025 20:26
@fuhry fuhry requested a review from maraino February 25, 2025 20:27
@maraino
Copy link
Collaborator

maraino commented Feb 25, 2025

  • If "user" is made a sub-key under "step", you end up with excessive object depth and resulting template complexity, e.g. {{if hasKey .Token, "step"}} {{if hasKey .Token.step "user"}} {{if hasKey .Token.step.user, "custom_key"}}. Golang text/template doesn't do lazy evaluation so if the user tries to combine these with and, the template won't run.

So I think it's best to keep step and user separated.

Sure, let's keep using user for now, @hslatman are you ok with this?

@hslatman
Copy link
Member

hslatman commented Feb 26, 2025

Having them in their own namespace sounds OK to me, and I think .Token.user signals sufficiently that they're provided by the user at token signing time.

The thing that could confuse users is that the --set and --set-file flags are also available to step ca certificate (and similar/related commands), and if they're used in that command, the properties are in the .Insecure.User namespace. In this case I presume the assumption is that the creator of the token controls the signing key, and is thus authorized to set a value. This then implies that the values can be considered "secure": they're coming from a trusted source. If we add this to documentation, we should point this out clearly.

Considering the previous paragraph, it could help if the namespace is not .Token.user, but then it wouldn't be immediately clear that they're user provided 😅

@fuhry
Copy link
Contributor Author

fuhry commented Feb 27, 2025

Ahh yes, I love a good naming debate! 😅

There are only two hard things in Computer Science: cache invalidation and naming things.
-Phil Karlton

@hslatman I'll add clarification to the step ca token subcommand's usage docs.

Is the capitalization difference something we want to address? This is particularly technically-incorrect here, as JWT claims are nominally all-lowercase, so we are faced with several choices, all not great:

  1. Keep as-is and explicitly document/tolerate the capitalization inconsistency between .Insecure.User and .Token.user
  2. Break standards and name the claim User (if that's even allowed?)
  3. Rename the claim to something else. Perhaps extra?

maraino
maraino previously approved these changes Mar 3, 2025
Copy link
Collaborator

@maraino maraino left a comment

Choose a reason for hiding this comment

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

Thanks, @fuhry. This looks good to me.

Let me confirm internally if we need any changes before merging this.

Add the `--set` and `--set-file` flags to the `step ca token` command,
allowing the user to set keys in the "user" claim in the resulting JWT.

Signed-off-by: Dan Fuhry <[email protected]>
@fuhry
Copy link
Contributor Author

fuhry commented Mar 3, 2025

Thanks, I've pushed a small change to the docs with the aforementioned clarification.

@hslatman hslatman added this to the v0.28.4 milestone Mar 3, 2025
@maraino maraino merged commit b1cff60 into smallstep:master Mar 4, 2025
12 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage Waiting for discussion / prioritization by team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants