-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Move audit filter before authn to log those failures as well #14535
Conversation
@mpbarrett fyi |
And it even looks easy to backport... :) |
So if we do this are we loosing the info from the user that is loaded during authentication? |
Looks like. I wonder whether we can't have both: put a fallback filter before authn, but keep the full-featured one where it is. If the later does not trigger (we can use a context value to know that), the fallback one will log the authn error. We haven't thought about this in advanced auditing either, but there we carry around the audit event in the context already. |
Yeah I was thinking along those lines as well. Full featured, then catch
all in fallback.
…On Fri, Jun 9, 2017 at 1:55 AM, Dr. Stefan Schimanski < ***@***.***> wrote:
So if we do this are we loosing the info from the user that is loaded
during authentication?
Looks like.
I wonder whether we can't have both: put a fallback filter before authn,
but keep the full-featured one where it is. If the later does not trigger
(we can use a context value to know that), the fallback one will log the
authn error.
We haven't thought about this in advanced auditing either, but there we
carry around the audit event in the context already.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#14535 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_p1JCz7VF49NqampDLzLR60cPN0ojks5sCN4-gaJpZM4N0nZ0>
.
|
@sttts @smarterclayton I've pushed current state with proposed changes from you guys, but it looks like I'm currently getting a bit more requests than I expected. Checkout this output: https://paste.fedoraproject.org/paste/ZZwM0dB7YQLypbXVqq5qiw and search for lines with |
Nvmd wrt to those double lines, it looks like my flag isn't working as I expected it to be :/ |
Of course I forgot to update the context mapper with new context data, updated, now it should be good. I still need to update the user to be reasonable. |
pkg/cmd/server/origin/audit.go
Outdated
apirequest "k8s.io/apiserver/pkg/endpoints/request" | ||
) | ||
|
||
const AUDIT_TRIGGERED = "audittriggered" |
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.
do we capital case constants?
pkg/cmd/server/origin/audit.go
Outdated
func (a *auditResponseWriter) WriteHeader(code int) { | ||
ctx, ok := a.requestContextMapper.Get(a.req) | ||
if !ok { | ||
glog.Errorf("Unable to get RequestContextMapper and read context data!") |
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.
worth a panic
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.
we're returning 500 in the original audit, I'll do the same here.
pkg/cmd/server/origin/audit.go
Outdated
|
||
var _ http.ResponseWriter = &auditResponseWriter{} | ||
|
||
type auditResponseWriter struct { |
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.
a good comment would be nice. It doesn't sound like this is just the fallback for the upstream audit handler. Maybe call it authFallbackAuditResponseWriter?
pkg/cmd/server/origin/audit.go
Outdated
|
||
// withAuditWrapper decorates a http.Handler with audit logging information for all the | ||
// requests coming to the server. If out is nil, no decoration takes place. | ||
func withAuditWrapper(handler http.Handler, requestContextMapper apirequest.RequestContextMapper, out io.Writer) http.Handler { |
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.
in fact there is no relation between WithAudit and this, isn't there? Would prefer WithAuditTriggeredMarker or something like that and an additional WithAudit call in the handler chain builder
pkg/cmd/server/origin/audit.go
Outdated
|
||
// withAuthnAudit decorates a http.Handler with audit logging ONLY information | ||
// related to failed Authn requests. | ||
func withAuthnAudit(handler http.Handler, requestContextMapper apirequest.RequestContextMapper, out io.Writer) http.Handler { |
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.
WithAuthFallbackAudit
I've addressed @sttts comments. The remaining piece, I'm currently working on, is adding information about auth methods used for authentication. |
user, ok, err := authenticator.AuthenticateRequest(req) | ||
if err != nil || !ok { | ||
ctx = apirequest.WithValue(ctx, AuditAuthnUser, user) | ||
ctx = apirequest.WithValue(ctx, AuditAuthnError, err) |
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.
@smarterclayton I've tried this approach to get the user and error and return that in the fallback audit logs, but it doesn't work :/ @sttts proposed we change AuthenticateRequest
signature by extending its return type with additional []AuthenticationResult
which will provide the information about user and method, with possibility to extend that in the future. We'd upstream this into 1.8, obviously. The only problem is that it might require a bit more work, and we're just closing 3.6. The other approach is to duplicate the logic responsible for getting user information in the fallback audit, and for token requests we'd just say it's that. wdyt?
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.
[test] |
As long as it's reasonably reliable, yes
…On Wed, Jun 14, 2017 at 11:22 AM, OpenShift Bot ***@***.***> wrote:
continuous-integration/openshift-jenkins/test Running (
https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2219/)
(Base Commit: c7a38d7
<c7a38d7>
)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#14535 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_p5YEsHvHLkuI6qi_xnRum1lLqNlbks5sD_rKgaJpZM4N0nZ0>
.
|
Here's the current sample output:
|
pkg/cmd/server/origin/audit.go
Outdated
|
||
// getUsername returns username or information on the authn method being used. | ||
func getUsername(req *http.Request) string { | ||
auth := strings.TrimSpace(req.Header.Get("Authorization")) |
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.
@soltysh love it. |
pkg/cmd/server/origin/audit.go
Outdated
} | ||
|
||
// other tokens | ||
token := strings.TrimSpace(req.URL.Query().Get("access_token")) |
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.
logging URLs before the auth filter can expose the query params
pkg/cmd/server/origin/audit.go
Outdated
} | ||
|
||
// check basic auth | ||
const basicScheme string = "Basic " |
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.
we don't support basic auth to our API, what is making use of this?
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.
Wasn't sure if we support or not, but I found the code in origin, so I've implemented it also here.
pkg/cmd/server/origin/audit.go
Outdated
if len(parts) > 1 && strings.ToLower(parts[0]) == "bearer" { | ||
token := parts[1] | ||
// Empty bearer tokens aren't valid | ||
if len(token) == 0 { |
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.
doing this to detect Authorization: Bearer
doesn't seem very useful
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.
it also draws a false distinction between empty bearer tokens and present but invalid bearer tokens
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.
I've tried with wrong and empty Bearer token and haven't seen any difference, I'll leave just the bearer check and will go with that.
pkg/cmd/server/origin/audit.go
Outdated
} | ||
|
||
// getUsername returns username or information on the authn method being used. | ||
func getUsername(req *http.Request) string { |
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.
this doesn't account for client cert auth
pkg/cmd/server/origin/audit.go
Outdated
return | ||
} | ||
id := uuid.NewRandom().String() | ||
line := fmt.Sprintf("%s AUDIT: id=%q ip=%q method=%q user=%q uri=%q\n", |
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.
presenting the results of getUsername() as user=...
seems wrong... it can attribute a request to a specific user pre-authentication, when it should be clearer that this is information the user presented that has not been verified in any way
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.
What else are you proposing?
pkg/cmd/server/origin/audit.go
Outdated
} | ||
|
||
// cert authn | ||
if req.TLS != nil && len(req.TLS.PeerCertificates) > 0 { |
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.
this will be confused by an auth proxy that presents a client cert and communicates user info in headers
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.
How can I fix this?
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2314/) (Base Commit: 4aa3720) (PR Branch Commit: 33007d2) |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: soltysh Assign the PR to them by writing No associated issue. Update pull-request body to add a reference to an issue, or get approval with The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: soltysh Assign the PR to them by writing No associated issue. Update pull-request body to add a reference to an issue, or get approval with The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: soltysh Assign the PR to them by writing No associated issue. Update pull-request body to add a reference to an issue, or get approval with The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
7442b22
to
b9872bc
Compare
pkg/cmd/server/origin/audit.go
Outdated
} | ||
|
||
// if there already exists an audit event in the context we don't need to do anything | ||
if ae := request.AuditEventFrom(ctx); ae != nil { |
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.
that's a lot of copied code. Don't like this. What are our options?
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.
Yeah, me neither. I was thinking about making this whole thing a helper function in audit upstream, wdyt?
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.
What was the reason to not do the same as this PR in upstream?
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.
Not sure, I think I can give it a try.
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.
If we get rid of maintaining our fork, go for it.
This is waiting for upstream PR kubernetes/kubernetes#51119 |
@soltysh PR needs rebase |
I'm closing this in favor of #16128 where I'll include the changes from this PR. |
@mfojtik @smarterclayton this change is logging authn failures in audit, this is the sample output:
@sttts @liggitt @deads2k fyi