-
Notifications
You must be signed in to change notification settings - Fork 4
SRVOCF-1078: Reduce deploy ServiceAccount token lifetime to a configurable 30 days #174
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2,8 +2,12 @@ package cluster | |||||
|
|
||||||
| import ( | ||||||
| "context" | ||||||
| "errors" | ||||||
| "fmt" | ||||||
| "log/slog" | ||||||
| "strconv" | ||||||
| "strings" | ||||||
| "time" | ||||||
|
|
||||||
| "github.com/openshift/faas-console-plugin/backend/kube" | ||||||
| authenticationv1 "k8s.io/api/authentication/v1" | ||||||
|
|
@@ -31,15 +35,62 @@ type Client interface { | |||||
| RequestToken(ctx context.Context, namespace string) (string, error) | ||||||
| } | ||||||
|
|
||||||
| // DefaultTokenExpiry is the requested SA token lifetime in seconds. Matches the | ||||||
| // previous frontend behaviour. Security concern: a long-lived token in an SCM | ||||||
| // Actions secret increases exposure if leaked; shorter expiry is a follow-up. | ||||||
| const DefaultTokenExpiry int64 = 365 * 24 * 60 * 60 // 1 year | ||||||
| // DefaultTokenExpiry is the requested SA token lifetime in seconds when no | ||||||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should not be exported. Pls set to private. Can you rename it to |
||||||
|
|
||||||
| // New creates a cluster client authenticated with token. | ||||||
| // errExpiryFormat describes the accepted SA_TOKEN_EXPIRY notation. | ||||||
| var errExpiryFormat = errors.New("must be a duration such as 30d, 10h, or 7d12h") | ||||||
|
|
||||||
| // ParseTokenExpiry converts the SA_TOKEN_EXPIRY value into a token lifetime in | ||||||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd add
Suggested change
|
||||||
| if s == "" { | ||||||
| return DefaultTokenExpiry, nil | ||||||
| } | ||||||
| d, err := parseExpiryDuration(s) | ||||||
| if err != nil { | ||||||
| return 0, fmt.Errorf("invalid token expiry %q: %w", s, err) | ||||||
| } | ||||||
| secs := int64(d / time.Second) | ||||||
| if secs <= 0 { | ||||||
| return 0, fmt.Errorf("invalid token expiry %q: must be at least one second", s) | ||||||
| } | ||||||
| return secs, nil | ||||||
| } | ||||||
|
|
||||||
| // parseExpiryDuration parses a duration that may include a leading days | ||||||
| // component (e.g. 7d or 7d12h). time.ParseDuration handles h/m/s but not d. | ||||||
| func parseExpiryDuration(s string) (time.Duration, error) { | ||||||
| var total time.Duration | ||||||
| rest := s | ||||||
| if i := strings.IndexByte(rest, 'd'); i >= 0 { | ||||||
| days, err := strconv.Atoi(rest[:i]) | ||||||
| if err != nil { | ||||||
| return 0, errExpiryFormat | ||||||
| } | ||||||
| total += time.Duration(days) * 24 * time.Hour | ||||||
| rest = rest[i+1:] | ||||||
| } | ||||||
| if rest != "" { | ||||||
| d, err := time.ParseDuration(rest) | ||||||
| if err != nil { | ||||||
| return 0, errExpiryFormat | ||||||
| } | ||||||
| total += d | ||||||
| } | ||||||
| return total, nil | ||||||
| } | ||||||
|
|
||||||
| // New creates a cluster client authenticated with token. tokenExpiry is the | ||||||
| // requested SA token lifetime in seconds (see ParseTokenExpiry). | ||||||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One could think here that tokenExpiry is for the token which is also passed but it's not. Can you rename:
|
||||||
| cfg, err := kube.RESTConfig(host, token, caCert) | ||||||
| if err != nil { | ||||||
| return nil, err | ||||||
|
|
@@ -49,11 +100,12 @@ func New(host, token string, caCert []byte) (Client, error) { | |||||
| if err != nil { | ||||||
| return nil, fmt.Errorf("create kubernetes client: %w", err) | ||||||
| } | ||||||
| return &k8sClient{clientset: clientset}, nil | ||||||
| return &k8sClient{clientset: clientset, tokenExpiry: tokenExpiry}, nil | ||||||
| } | ||||||
|
|
||||||
| type k8sClient struct { | ||||||
| clientset kubernetes.Interface | ||||||
| clientset kubernetes.Interface | ||||||
| tokenExpiry int64 // requested SA token lifetime in seconds | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the name is pretty clear, even more so with SA -> |
||||||
| } | ||||||
|
|
||||||
| func (c *k8sClient) CreateServiceAccount(ctx context.Context, namespace string) (bool, error) { | ||||||
|
|
@@ -181,7 +233,7 @@ func (c *k8sClient) DeleteImageBuilderBinding(ctx context.Context, namespace str | |||||
| } | ||||||
|
|
||||||
| func (c *k8sClient) RequestToken(ctx context.Context, namespace string) (string, error) { | ||||||
| expiry := DefaultTokenExpiry | ||||||
| expiry := c.tokenExpiry | ||||||
| result, err := c.clientset.CoreV1().ServiceAccounts(namespace).CreateToken(ctx, saName, &authenticationv1.TokenRequest{ | ||||||
| Spec: authenticationv1.TokenRequestSpec{ | ||||||
| ExpirationSeconds: &expiry, | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -369,6 +369,28 @@ var _ = Describe("Kubernetes cluster client", func() { | |
| Expect(token).To(Equal("sa-token-value")) | ||
| }) | ||
|
|
||
| It("requests a token with the configured expiry", func() { | ||
| var requestedExpiry *int64 | ||
| cs := fake.NewSimpleClientset() | ||
| cs.PrependReactor("create", "serviceaccounts", func(action k8stesting.Action) (bool, runtime.Object, error) { | ||
| if action.GetSubresource() != "token" { | ||
| return false, nil, nil | ||
| } | ||
| tr := action.(k8stesting.CreateAction).GetObject().(*authenticationv1.TokenRequest) | ||
| requestedExpiry = tr.Spec.ExpirationSeconds | ||
| return true, &authenticationv1.TokenRequest{ | ||
| Status: authenticationv1.TokenRequestStatus{Token: "sa-token-value"}, | ||
| }, nil | ||
| }) | ||
| cl := &k8sClient{clientset: cs, tokenExpiry: 7 * 24 * 60 * 60} | ||
|
|
||
| _, err := cl.RequestToken(context.Background(), "default") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test will need an update. |
||
|
|
||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(requestedExpiry).NotTo(BeNil()) | ||
| Expect(*requestedExpiry).To(Equal(int64(7 * 24 * 60 * 60))) | ||
| }) | ||
|
|
||
| It("returns an error when the token endpoint is unavailable", func() { | ||
| cs := fake.NewSimpleClientset() | ||
| cs.PrependReactor("create", "serviceaccounts", func(action k8stesting.Action) (bool, runtime.Object, error) { | ||
|
|
@@ -385,3 +407,37 @@ var _ = Describe("Kubernetes cluster client", func() { | |
| }) | ||
| }) | ||
| }) | ||
|
|
||
| var _ = Describe("ParseTokenExpiry", func() { | ||
| It("defaults to 30 days when unset", func() { | ||
| secs, err := ParseTokenExpiry("") | ||
|
|
||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(secs).To(Equal(int64(30 * 24 * 60 * 60))) | ||
| }) | ||
|
|
||
| DescribeTable("parses common duration notation", | ||
| func(value string, want int64) { | ||
| secs, err := ParseTokenExpiry(value) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(secs).To(Equal(want)) | ||
| }, | ||
| Entry("days", "7d", int64(7*24*60*60)), | ||
| Entry("hours", "10h", int64(10*60*60)), | ||
| Entry("minutes", "90m", int64(90*60)), | ||
| Entry("days and hours combined", "7d12h", int64(7*24*60*60+12*60*60)), | ||
| ) | ||
|
|
||
| DescribeTable("rejects invalid values", | ||
| func(value string) { | ||
| _, err := ParseTokenExpiry(value) | ||
| Expect(err).To(HaveOccurred()) | ||
| }, | ||
| Entry("non-numeric", "banana"), | ||
| Entry("missing unit", "720"), | ||
| Entry("bare days unit", "d"), | ||
| Entry("zero", "0h"), | ||
| Entry("negative", "-5h"), | ||
| Entry("rounds down to zero seconds", "500ms"), | ||
| ) | ||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Token expiry is a parameter of
RequestToken, one of the cluster client's operations. It doesn't belong on the client struct itself.Move
saTokenExpiryout ofk8sClientand into the method signatures:cluster.Newstays as it was (nosaTokenExpiryparameter),k8sClientdoesn't store it:The env var should be
DEFAULT_SA_TOKEN_EXPIRY, parsed at startup and stored on theHandlersstruct:The handler resolves the effective expiry and passes it through:
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.