SRVOCF-1078: Reduce deploy ServiceAccount token lifetime to a configurable 30 days - #174
Conversation
The deploy ServiceAccount token was minted with a 365 day (1 year) lifetime and stored in a GitHub Actions secret. A credential that long-lived is unnecessary exposure if the secret leaks, since it far outlasts the interval between deployments. Reduce the default lifetime to 30 days and make it configurable via the SA_TOKEN_EXPIRY env var, accepting common duration notation such as 30d, 10h, or 7d12h. The value is wired through a Helm value (plugin.saTokenExpiry) into the deployment's container env. The token is still a one-shot credential with no in-product refresh, so a function's CI stops deploying once the token expires. A user-triggered re-issue path and keyless rotation are tracked as follow-ups. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@dsimansk: This pull request references SRVOCF-1078 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "5.1.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@dsimansk: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
twoGiants
left a comment
twoGiants
left a comment
There was a problem hiding this comment.
Alright, looks good!
Adding the SA token expiration as a configuration via env vars is a good call.
I'd remove the tokenExpiration from the client struct -> it doesn't belong there, it's an option for RequestToken not a config parameter of the k8 client.
On Tuesday I talked with Finn about it -> we will have an Admin dashboard at some point from where such things like token expiration will be configured.
| // override is configured. Kept short to limit exposure of the token stored in | ||
| // an SCM Actions secret if leaked. Override via the SA_TOKEN_EXPIRY env var | ||
| // (a duration such as 30d, 10h, or 7d12h); see ParseTokenExpiry. | ||
| const DefaultTokenExpiry int64 = 30 * 24 * 60 * 60 // 30 days |
There was a problem hiding this comment.
Should not be exported. Pls set to private.
Can you rename it to DefaultSATokenExpiry and add the SA letters to all the variables and parameters for the token? Thx!
| // seconds. The value is a duration in common notation, e.g. 30d, 10h, or | ||
| // 7d12h. The 'd' (days) unit extends Go's standard duration units (h, m, s). | ||
| // An empty value yields DefaultTokenExpiry. | ||
| func ParseTokenExpiry(s string) (int64, error) { |
There was a problem hiding this comment.
I'd add SA to the name.
| func ParseTokenExpiry(s string) (int64, error) { | |
| func ParseSATokenExpiry(s string) (int64, error) { |
| type k8sClient struct { | ||
| clientset kubernetes.Interface | ||
| clientset kubernetes.Interface | ||
| tokenExpiry int64 // requested SA token lifetime in seconds |
There was a problem hiding this comment.
I think the name is pretty clear, even more so with SA -> saTokenExpiry, so no comment is needed.
| // When host is non-empty (dev/test) it is used as the API server URL directly. | ||
| // When host is empty the standard in-cluster config is used (pod env vars + SA files). | ||
| func New(host, token string, caCert []byte) (Client, error) { | ||
| func New(host, token string, caCert []byte, tokenExpiry int64) (Client, error) { |
There was a problem hiding this comment.
One could think here that tokenExpiry is for the token which is also passed but it's not.
Can you rename:
- token -> restToken
- tokenExpiry -> saTokenExpiry (as proposed above)
There was a problem hiding this comment.
Token expiry is a parameter of RequestToken, one of the cluster client's operations. It doesn't belong on the client struct itself.
Move saTokenExpiry out of k8sClient and into the method signatures:
// cluster/client.go
RequestToken(ctx context.Context, namespace string, saTokenExpiry int64) (string, error)
// cluster/kubeconfig.go
func GenerateKubeconfig(ctx context.Context, client Client, namespace, externalAPIServerURL string, caCert []byte, saTokenExpiry int64) (string, error)cluster.New stays as it was (no saTokenExpiry parameter), k8sClient doesn't store it:
func New(host, token string, caCert []byte) (Client, error)The env var should be DEFAULT_SA_TOKEN_EXPIRY, parsed at startup and stored on the Handlers struct:
// main.go
saTokenExpiry, err := cluster.ParseSATokenExpiry(os.Getenv("DEFAULT_SA_TOKEN_EXPIRY"))
// handler/handler.go
type Handlers struct {
// ...existing fields...
defaultSATokenExpiry int64 // fallback SA token lifetime; only used when the create request omits saTokenExpiry
}The handler resolves the effective expiry and passes it through:
func (h *Handlers) resolveSATokenExpiry(requested string) (int64, error) {
if requested == "" {
return h.defaultSATokenExpiry, nil
}
return cluster.ParseSATokenExpiry(requested)
}
// in createFunction:
cl, err := newClusterClient(h.kubeHost, ocpToken, h.caCert)
// ...
kubeconfig, err := cluster.GenerateKubeconfig(ctx, cl, req.Namespace, h.externalAPIServerURL, h.caCert, h.defaultSATokenExpiry)Helm value rename accordingly (plugin.defaultSaTokenExpiry) with a comment that it's the fallback when the create request doesn't provide one.
At some point when we will probably have an admin dashboard and we will read the saTokenExpiry in the backend from some config which we created from the admin dashboard.
| }) | ||
| cl := &k8sClient{clientset: cs, tokenExpiry: 7 * 24 * 60 * 60} | ||
|
|
||
| _, err := cl.RequestToken(context.Background(), "default") |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
3 participants
Summary
(1 year) to 30 days, shrinking the exposure window for the credential
stored in the repo's GitHub Actions KUBECONFIG secret.
accepting common duration notation (30d, 10h, 7d12h). Empty falls back
to the 30 day default. Exposed as the plugin.saTokenExpiry Helm value
and wired into the deployment container env.
RequestToken; add unit tests for the parser and for the configured
expiry reaching the TokenRequest.
Fixes SRVOCF-1078
Checklist
docs/ARCHITECTURE.md(if there are relevant changes to our layered architecture)Additional Info
The token is still a one-shot credential with no in-product refresh, so a
function's CI stops deploying once the token expires. A user-triggered
re-issue path and keyless rotation (GitHub OIDC federation or a GitOps
pull model) are tracked as follow-ups on the Jira issue; they are out of
scope here.