Skip to content

Commit 5125310

Browse files
liggittliggitt
committed
Consider future deletionTimestamps when validating bound tokens
1 parent 894916b commit 5125310

File tree

3 files changed

+175
-4
lines changed

3 files changed

+175
-4
lines changed

‎pkg/serviceaccount/BUILD‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,9 @@ go_test(
5757
"//pkg/controller/serviceaccount:go_default_library",
5858
"//pkg/routes:go_default_library",
5959
"//staging/src/k8s.io/api/core/v1:go_default_library",
60+
"//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library",
6061
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
62+
"//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
6163
"//staging/src/k8s.io/apiserver/pkg/authentication/authenticator:go_default_library",
6264
"//staging/src/k8s.io/client-go/kubernetes:go_default_library",
6365
"//staging/src/k8s.io/client-go/kubernetes/fake:go_default_library",

‎pkg/serviceaccount/claims.go‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -98,8 +98,9 @@ func (v *validator) Validate(_ string, public *jwt.Claims, privateObj interface{
9898
klog.Errorf("jwt validator expected private claim of type *privateClaims but got: %T", privateObj)
9999
return nil, errors.New("Token could not be validated.")
100100
}
101+
nowTime := now()
101102
err := public.Validate(jwt.Expected{
102-
Time: now(),
103+
Time: nowTime,
103104
})
104105
switch {
105106
case err == nil:
@@ -110,6 +111,8 @@ func (v *validator) Validate(_ string, public *jwt.Claims, privateObj interface{
110111
return nil, errors.New("Token could not be validated.")
111112
}
112113

114+
// consider things deleted prior to now()-leeway to be invalid
115+
invalidIfDeletedBefore := nowTime.Add(-jwt.DefaultLeeway)
113116
namespace := private.Kubernetes.Namespace
114117
saref := private.Kubernetes.Svcacct
115118
podref := private.Kubernetes.Pod
@@ -120,7 +123,7 @@ func (v *validator) Validate(_ string, public *jwt.Claims, privateObj interface{
120123
klog.V(4).Infof("Could not retrieve service account %s/%s: %v", namespace, saref.Name, err)
121124
return nil, err
122125
}
123-
if serviceAccount.DeletionTimestamp != nil {
126+
if serviceAccount.DeletionTimestamp != nil && serviceAccount.DeletionTimestamp.Time.Before(invalidIfDeletedBefore) {
124127
klog.V(4).Infof("Service account has been deleted %s/%s", namespace, saref.Name)
125128
return nil, fmt.Errorf("ServiceAccount %s/%s has been deleted", namespace, saref.Name)
126129
}
@@ -136,7 +139,7 @@ func (v *validator) Validate(_ string, public *jwt.Claims, privateObj interface{
136139
klog.V(4).Infof("Could not retrieve bound secret %s/%s for service account %s/%s: %v", namespace, secref.Name, namespace, saref.Name, err)
137140
return nil, errors.New("Token has been invalidated")
138141
}
139-
if secret.DeletionTimestamp != nil {
142+
if secret.DeletionTimestamp != nil && secret.DeletionTimestamp.Time.Before(invalidIfDeletedBefore) {
140143
klog.V(4).Infof("Bound secret is deleted and awaiting removal: %s/%s for service account %s/%s", namespace, secref.Name, namespace, saref.Name)
141144
return nil, errors.New("Token has been invalidated")
142145
}
@@ -154,7 +157,7 @@ func (v *validator) Validate(_ string, public *jwt.Claims, privateObj interface{
154157
klog.V(4).Infof("Could not retrieve bound pod %s/%s for service account %s/%s: %v", namespace, podref.Name, namespace, saref.Name, err)
155158
return nil, errors.New("Token has been invalidated")
156159
}
157-
if pod.DeletionTimestamp != nil {
160+
if pod.DeletionTimestamp != nil && pod.DeletionTimestamp.Time.Before(invalidIfDeletedBefore) {
158161
klog.V(4).Infof("Bound pod is deleted and awaiting removal: %s/%s for service account %s/%s", namespace, podref.Name, namespace, saref.Name)
159162
return nil, errors.New("Token has been invalidated")
160163
}

‎pkg/serviceaccount/claims_test.go‎

Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,10 @@ import (
2424

2525
"gopkg.in/square/go-jose.v2/jwt"
2626

27+
v1 "k8s.io/api/core/v1"
28+
apierrors "k8s.io/apimachinery/pkg/api/errors"
2729
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
30+
"k8s.io/apimachinery/pkg/runtime/schema"
2831
"k8s.io/kubernetes/pkg/apis/core"
2932
)
3033

@@ -182,3 +185,166 @@ func TestClaims(t *testing.T) {
182185
})
183186
}
184187
}
188+
189+
type deletionTestCase struct {
190+
name string
191+
time *metav1.Time
192+
expectErr bool
193+
}
194+
195+
type claimTestCase struct {
196+
name string
197+
getter ServiceAccountTokenGetter
198+
private *privateClaims
199+
expectErr bool
200+
}
201+
202+
func TestValidatePrivateClaims(t *testing.T) {
203+
var (
204+
nowUnix = int64(1514764800)
205+
206+
serviceAccount = &v1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Name: "saname", Namespace: "ns", UID: "sauid"}}
207+
secret = &v1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "secretname", Namespace: "ns", UID: "secretuid"}}
208+
pod = &v1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "podname", Namespace: "ns", UID: "poduid"}}
209+
)
210+
211+
deletionTestCases := []deletionTestCase{
212+
{
213+
name: "valid",
214+
time: nil,
215+
},
216+
{
217+
name: "deleted now",
218+
time: &metav1.Time{Time: time.Unix(nowUnix, 0)},
219+
},
220+
{
221+
name: "deleted near past",
222+
time: &metav1.Time{Time: time.Unix(nowUnix-1, 0)},
223+
},
224+
{
225+
name: "deleted near future",
226+
time: &metav1.Time{Time: time.Unix(nowUnix+1, 0)},
227+
},
228+
{
229+
name: "deleted now-leeway",
230+
time: &metav1.Time{Time: time.Unix(nowUnix-60, 0)},
231+
},
232+
{
233+
name: "deleted now-leeway-1",
234+
time: &metav1.Time{Time: time.Unix(nowUnix-61, 0)},
235+
expectErr: true,
236+
},
237+
}
238+
239+
testcases := []claimTestCase{
240+
{
241+
name: "missing serviceaccount",
242+
getter: fakeGetter{nil, nil, nil},
243+
private: &privateClaims{Kubernetes: kubernetes{Svcacct: ref{Name: "saname", UID: "sauid"}, Namespace: "ns"}},
244+
expectErr: true,
245+
},
246+
{
247+
name: "missing secret",
248+
getter: fakeGetter{serviceAccount, nil, nil},
249+
private: &privateClaims{Kubernetes: kubernetes{Svcacct: ref{Name: "saname", UID: "sauid"}, Secret: &ref{Name: "secretname", UID: "secretuid"}, Namespace: "ns"}},
250+
expectErr: true,
251+
},
252+
{
253+
name: "missing pod",
254+
getter: fakeGetter{serviceAccount, nil, nil},
255+
private: &privateClaims{Kubernetes: kubernetes{Svcacct: ref{Name: "saname", UID: "sauid"}, Pod: &ref{Name: "podname", UID: "poduid"}, Namespace: "ns"}},
256+
expectErr: true,
257+
},
258+
{
259+
name: "different uid serviceaccount",
260+
getter: fakeGetter{serviceAccount, nil, nil},
261+
private: &privateClaims{Kubernetes: kubernetes{Svcacct: ref{Name: "saname", UID: "sauidold"}, Namespace: "ns"}},
262+
expectErr: true,
263+
},
264+
{
265+
name: "different uid secret",
266+
getter: fakeGetter{serviceAccount, secret, nil},
267+
private: &privateClaims{Kubernetes: kubernetes{Svcacct: ref{Name: "saname", UID: "sauid"}, Secret: &ref{Name: "secretname", UID: "secretuidold"}, Namespace: "ns"}},
268+
expectErr: true,
269+
},
270+
{
271+
name: "different uid pod",
272+
getter: fakeGetter{serviceAccount, nil, pod},
273+
private: &privateClaims{Kubernetes: kubernetes{Svcacct: ref{Name: "saname", UID: "sauid"}, Pod: &ref{Name: "podname", UID: "poduidold"}, Namespace: "ns"}},
274+
expectErr: true,
275+
},
276+
}
277+
278+
for _, deletionTestCase := range deletionTestCases {
279+
var (
280+
deletedServiceAccount = serviceAccount.DeepCopy()
281+
deletedPod = pod.DeepCopy()
282+
deletedSecret = secret.DeepCopy()
283+
)
284+
deletedServiceAccount.DeletionTimestamp = deletionTestCase.time
285+
deletedPod.DeletionTimestamp = deletionTestCase.time
286+
deletedSecret.DeletionTimestamp = deletionTestCase.time
287+
288+
testcases = append(testcases,
289+
claimTestCase{
290+
name: deletionTestCase.name + " serviceaccount",
291+
getter: fakeGetter{deletedServiceAccount, nil, nil},
292+
private: &privateClaims{Kubernetes: kubernetes{Svcacct: ref{Name: "saname", UID: "sauid"}, Namespace: "ns"}},
293+
expectErr: deletionTestCase.expectErr,
294+
},
295+
claimTestCase{
296+
name: deletionTestCase.name + " secret",
297+
getter: fakeGetter{serviceAccount, deletedSecret, nil},
298+
private: &privateClaims{Kubernetes: kubernetes{Svcacct: ref{Name: "saname", UID: "sauid"}, Secret: &ref{Name: "secretname", UID: "secretuid"}, Namespace: "ns"}},
299+
expectErr: deletionTestCase.expectErr,
300+
},
301+
claimTestCase{
302+
name: deletionTestCase.name + " pod",
303+
getter: fakeGetter{serviceAccount, nil, deletedPod},
304+
private: &privateClaims{Kubernetes: kubernetes{Svcacct: ref{Name: "saname", UID: "sauid"}, Pod: &ref{Name: "podname", UID: "poduid"}, Namespace: "ns"}},
305+
expectErr: deletionTestCase.expectErr,
306+
},
307+
)
308+
}
309+
310+
for _, tc := range testcases {
311+
t.Run(tc.name, func(t *testing.T) {
312+
v := &validator{tc.getter}
313+
_, err := v.Validate("", &jwt.Claims{Expiry: jwt.NumericDate(nowUnix)}, tc.private)
314+
if err != nil && !tc.expectErr {
315+
t.Fatal(err)
316+
}
317+
if err == nil && tc.expectErr {
318+
t.Fatal("expected error, got none")
319+
}
320+
if err != nil {
321+
return
322+
}
323+
})
324+
}
325+
}
326+
327+
type fakeGetter struct {
328+
serviceAccount *v1.ServiceAccount
329+
secret *v1.Secret
330+
pod *v1.Pod
331+
}
332+
333+
func (f fakeGetter) GetServiceAccount(namespace, name string) (*v1.ServiceAccount, error) {
334+
if f.serviceAccount == nil {
335+
return nil, apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "serviceaccounts"}, name)
336+
}
337+
return f.serviceAccount, nil
338+
}
339+
func (f fakeGetter) GetPod(namespace, name string) (*v1.Pod, error) {
340+
if f.pod == nil {
341+
return nil, apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "pods"}, name)
342+
}
343+
return f.pod, nil
344+
}
345+
func (f fakeGetter) GetSecret(namespace, name string) (*v1.Secret, error) {
346+
if f.secret == nil {
347+
return nil, apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "secrets"}, name)
348+
}
349+
return f.secret, nil
350+
}

0 commit comments

Comments
 (0)