WI: interpolate parent job ID in vault.default_identity.extra_claims
#23817
Conversation
tgross
force-pushed
the
b-interpolate-claims-job-parent-id
branch
from
f4d730e to
fc05cbe
Compare
tgross
added
theme/vault
type/bug
theme/batch
tgross
force-pushed
the
b-interpolate-claims-job-parent-id
branch
from
fc05cbe to
378d4b3
Compare
tgross
changed the title
vault.default_identity.extra_claims
| @@ -200,7 +201,7 @@ func TestNewIdentityClaims(t *testing.T) { | |||
| }, | |||
| "job/group/task/alt-identity": { | |||
| Namespace: "default", | |||
| JobID: "job", | |||
| JobID: "parentJob", | |||
| TaskName: "task", | |||
| Claims: jwt.Claims{ | |||
| Subject: "global:default:job:group:task:alt-identity", | |||
There was a problem hiding this comment.
@schmichael I've just now noticed we don't use the parent ID for the Subject claim... is that intentional or did we repeat the same bug there?
There was a problem hiding this comment.
We repeated the same bug. I'm sure I just forgot about the distinction between Parent/Child IDs when writing it initially. :(
There was a problem hiding this comment.
I'll fix that in a separate PR.
There was a problem hiding this comment.
Even though I cannot come up with an actual use-case, there might be instances where someone wants to use the /periodic-123456 suffix as part of their claim. Or something else somewhat unique to the individual run (e.g. allocation ID).
Use-cases will probably be related to the fact that the field you select in Vault for the user_claim needs to be unique for the combination of claims. For example:
Before Nomad v1.8.3 you had to configure /sub as user_claim if you wanted different dynamic database credentials (Vault database secret engine) per-task within the same Nomad job. For example one migrations-tasks with full admin rights to the databse and one webserver-task with limited permissions. If you configured something else as the user_claim, e.g. nomad_job_id, then the tasks would 'overwrite' each other, pointing to the wrong JWT claims. I could imagine something similar happening to someone when it comes to unique invocations of periodic/batch jobs, providing a need for something unique-ish - be that alloc.id or the /periodic-123456 suffix.
Whether that's just an additional interpolate variable or whether it's part of the Subject claim isn't up to me, but providing something somewhere that is unique to the allocation/run might be beneficial. As of right now, it'd only be available as one of the builtin claims and not part of anything you can interpolate into extra claims.
When we interpolate job fields for the `vault.default_identity.extra_claims` block, we forgot to use the parent job ID when that's available (as we do for all other claims). This changeset fixes the bug and adds a helper method that'll hopefully remind us to do this going forward. Also added a missing changelog entry for #23675 where we implemented the `extra_claims` block originally, which shipped in Nomad 1.8.3. Fixes: #23798
|
tgross
force-pushed
the
b-interpolate-claims-job-parent-id
branch
from
378d4b3 to
c98e6c5
Compare
When we use the job ID in creating the subject claim (`sub`) for workload identities, we forgot to use the parent job ID when that's available. Child job IDs have a random component that makes them unsuitable for the subject field. Ref: #23817 (comment) Ref: https://hashicorp.atlassian.net/browse/NET-10714
When we use the job ID in creating the subject claim (`sub`) for workload identities, we forgot to use the parent job ID when that's available. Child job IDs have a random component that makes them unsuitable for the subject field. Ref: #23817 (comment) Ref: https://hashicorp.atlassian.net/browse/NET-10714
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
When we interpolate job fields for the
vault.default_identity.extra_claimsblock, we forgot to use the parent job ID when that's available (as we do for all other claims). This changeset fixes the bug and adds a helper method that'll hopefully remind us to do this going forward.Also added a missing changelog entry for #23675 where we implemented the
extra_claimsblock originally, which shipped in Nomad 1.8.3.Fixes: #23798
Ref: https://hashicorp.atlassian.net/browse/NET-10714