-
Notifications
You must be signed in to change notification settings - Fork 1
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
[endpoint] - Add more info in audit log #86
base: release-1.31-dd-v1.31.2-dd
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 |
---|---|---|
@@ -0,0 +1,47 @@ | ||
/* | ||
Copyright 2019 The Kubernetes Authors. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package filters | ||
|
||
import ( | ||
"crypto/tls" | ||
"k8s.io/apiserver/pkg/audit" | ||
"net/http" | ||
) | ||
|
||
// WithDDOGAudits adds additional information about the request to the audit logs. | ||
// This is useful for debugging and troubleshooting. | ||
// TLS Cipher for fips | ||
// Content-Type of the response to track JSON vs protobuf | ||
func WithDDOGAudits(handler http.Handler) http.Handler { | ||
return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { | ||
ctx := req.Context() | ||
|
||
// Set the content type annotation if it is set | ||
if _, ok := w.Header()["Content-Type"]; ok { | ||
audit.AddAuditAnnotation(ctx, "audit.datadoghq.com/contentType", w.Header().Get("Content-Type")) | ||
} | ||
|
||
// Set the TLS Cipher annotation if TLS and CipherSuite are set | ||
if req.TLS != nil { | ||
if req.TLS.CipherSuite > 0 { | ||
audit.AddAuditAnnotation(ctx, "audit.datadoghq.com/cipher", tls.CipherSuiteName(req.TLS.CipherSuite)) | ||
} | ||
} | ||
|
||
handler.ServeHTTP(w, req) | ||
}) | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,130 @@ | ||||||
package filters | ||||||
|
||||||
import ( | ||||||
"crypto/tls" | ||||||
"crypto/x509" | ||||||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||||||
auditinternal "k8s.io/apiserver/pkg/apis/audit" | ||||||
"k8s.io/apiserver/pkg/audit/policy" | ||||||
"k8s.io/apiserver/pkg/authentication/user" | ||||||
"k8s.io/apiserver/pkg/endpoints/request" | ||||||
"net/http" | ||||||
"net/http/httptest" | ||||||
"testing" | ||||||
) | ||||||
|
||||||
func TestWithDDOGAudits(t *testing.T) { | ||||||
handler := func(http.ResponseWriter, *http.Request) {} | ||||||
shortRunningPath := "/api/v1/namespaces/default/pods/foo" | ||||||
|
||||||
for _, test := range []struct { | ||||||
desc string | ||||||
tlsCipher uint16 | ||||||
contentType string | ||||||
expected []auditinternal.Event | ||||||
}{ | ||||||
{ | ||||||
"TLS Cipher set & Content Type set", | ||||||
tls.TLS_RSA_WITH_RC4_128_SHA, | ||||||
"application/json", | ||||||
[]auditinternal.Event{ | ||||||
{ | ||||||
Stage: auditinternal.StageResponseComplete, | ||||||
Verb: "get", | ||||||
RequestURI: shortRunningPath, | ||||||
ResponseStatus: &metav1.Status{Code: 200}, | ||||||
Annotations: map[string]string{ | ||||||
"audit.datadoghq.com/cipher": "TLS_RSA_WITH_RC4_128_SHA", | ||||||
"audit.datadoghq.com/contentType": "application/json", | ||||||
}, | ||||||
}, | ||||||
}, | ||||||
}, { | ||||||
"TLS Cipher set & Content Type unset", | ||||||
tls.TLS_RSA_WITH_RC4_128_SHA, | ||||||
"", | ||||||
[]auditinternal.Event{ | ||||||
{ | ||||||
Stage: auditinternal.StageResponseComplete, | ||||||
Verb: "get", | ||||||
RequestURI: shortRunningPath, | ||||||
ResponseStatus: &metav1.Status{Code: 200}, | ||||||
Annotations: map[string]string{ | ||||||
"audit.datadoghq.com/cipher": "TLS_RSA_WITH_RC4_128_SHA", | ||||||
}, | ||||||
}, | ||||||
}, | ||||||
}, { | ||||||
"TLS Cipher unset & Content Type unset", | ||||||
0, | ||||||
"", | ||||||
[]auditinternal.Event{ | ||||||
{ | ||||||
Stage: auditinternal.StageResponseComplete, | ||||||
Verb: "get", | ||||||
RequestURI: shortRunningPath, | ||||||
ResponseStatus: &metav1.Status{Code: 200}, | ||||||
}, | ||||||
}, | ||||||
}, { | ||||||
"TLS Cipher unset & Content Type set", | ||||||
0, | ||||||
"application/json", | ||||||
[]auditinternal.Event{ | ||||||
{ | ||||||
Stage: auditinternal.StageResponseComplete, | ||||||
Verb: "get", | ||||||
RequestURI: shortRunningPath, | ||||||
ResponseStatus: &metav1.Status{Code: 200}, | ||||||
Annotations: map[string]string{ | ||||||
"audit.datadoghq.com/contentType": "application/json", | ||||||
}, | ||||||
}, | ||||||
}, | ||||||
}, | ||||||
} { | ||||||
t.Run(test.desc, func(t *testing.T) { | ||||||
sink := &fakeAuditSink{} | ||||||
fakeRuleEvaluator := policy.NewFakePolicyRuleEvaluator(auditinternal.LevelRequestResponse, []auditinternal.Stage{auditinternal.StageRequestReceived, auditinternal.StageResponseStarted}) | ||||||
handler := WithAudit(http.HandlerFunc(handler), sink, fakeRuleEvaluator, func(r *http.Request, ri *request.RequestInfo) bool { return true }) | ||||||
handler = WithAuditInit(handler) | ||||||
handler = WithDDOGAudits(handler) | ||||||
|
||||||
req, _ := http.NewRequest("GET", shortRunningPath, nil) | ||||||
req.RemoteAddr = "127.0.0.1" | ||||||
req = withTestContext(req, &user.DefaultInfo{Name: "admin"}, nil) | ||||||
res := httptest.NewRecorder() | ||||||
|
||||||
if test.tlsCipher > 0 { | ||||||
req.TLS = &tls.ConnectionState{PeerCertificates: []*x509.Certificate{{}}, CipherSuite: test.tlsCipher} | ||||||
} | ||||||
if test.contentType != "" { | ||||||
res.Header().Set("Content-Type", test.contentType) | ||||||
} | ||||||
|
||||||
func() { | ||||||
defer func() { | ||||||
recover() | ||||||
}() | ||||||
handler.ServeHTTP(res, req) | ||||||
}() | ||||||
|
||||||
events := sink.Events() | ||||||
t.Logf("audit log: %v", events) | ||||||
if len(events) != len(test.expected) { | ||||||
t.Fatalf("Unexpected amount of lines in audit log: %d", len(events)) | ||||||
} | ||||||
for i, _ := range test.expected { | ||||||
event := events[i] | ||||||
if len(event.Annotations) != len(test.expected[i].Annotations) { | ||||||
t.Errorf("[%s] expected %d annotations, got %d", test.desc, len(test.expected[i].Annotations), len(event.Annotations)) | ||||||
} | ||||||
for y, _ := range event.Annotations { | ||||||
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. 🔵 Code Quality Violation
Suggested change
Unnecessary blank identifier (...read more)In Go, when using range iterations or receiving values from channels, it is recommended to avoid assigning the iteration or received value to the blank identifier Here's why it is best to use
For example, consider the following code snippets: for _ = range aSlice {}
x, _ = something()
_ = <- aChannel for range aSlice {}
x = something()
<-aChannel Both snippets achieve the same result, but the second one that omits the assignments using By using |
||||||
if test.expected[i].Annotations[y] != event.Annotations[y] { | ||||||
t.Errorf("[%s] expected annotation %s to be %s, got %s", test.desc, y, test.expected[i].Annotations[y], event.Annotations[y]) | ||||||
} | ||||||
} | ||||||
} | ||||||
}) | ||||||
} | ||||||
} |
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.
🔵 Code Quality Violation
Unnecessary blank identifier (...read more)
In Go, when using range iterations or receiving values from channels, it is recommended to avoid assigning the iteration or received value to the blank identifier
_
. Instead, it is preferred to omit the assignment entirely.Here's why it is best to use
for range s {}
,x = someMap[key]
, and<-ch
instead of using the blank identifier_
:_
can introduce confusion and make it less clear what the purpose of the assignment is or if the value is discarded intentionally or accidentally._
as an assignment can unnecessarily pollute the variable space. Although Go allows the use of the blank identifier_
to disregard a value, it is a good practice to avoid introducing unnecessary variables, especially if they are never used.varName = _
as an indication of accidental assignment or failure to handle errors or returned values properly. Removing these assignments eliminates such warnings or false-positive detections.For example, consider the following code snippets:
Both snippets achieve the same result, but the second one that omits the assignments using
_
is preferred for its simplicity, readability, and adherence to Go's best practices.By using
for range s {}
,x = someMap[key]
, and<-ch
instead of assigning to_
, you can write cleaner and more readable Go code while avoiding unnecessary variable assignments and potential confusion.