Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[advanced audit] fix invalid status code for hijacker #46854

Merged
merged 1 commit into from
Jun 6, 2017
Merged

[advanced audit] fix invalid status code for hijacker #46854

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion staging/src/k8s.io/apiserver/pkg/endpoints/filters/audit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ func (a *auditResponseWriter) processCode(code int) {
}

func (a *auditResponseWriter) Write(bs []byte) (int, error) {
a.processCode(200) // the Go library calls WriteHeader internally if no code was written yet. But this will go unnoticed for us
a.processCode(http.StatusOK) // the Go library calls WriteHeader internally if no code was written yet. But this will go unnoticed for us
return a.ResponseWriter.Write(bs)
}

Expand All @@ -208,6 +208,8 @@ func (f *fancyResponseWriterDelegator) Flush() {
}

func (f *fancyResponseWriterDelegator) Hijack() (net.Conn, *bufio.ReadWriter, error) {
// fake a response status before protocol switch happens
f.processCode(http.StatusSwitchingProtocols)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not all hijacks switch protocols... for example, streaming logs hijacks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which means that in the case of Hijacking we actually have no clue what will happen. The hijacker might write headers or another code, i.e. manually via "binary" write. The only thing we can do is to introduce another audit stage without any http code (similar to the panic stage). /cc @timstclair @soltysh @CaoShuFeng is it worth it to add that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

until we figure that out, please revert this change... setting 101 in non-switching cases is an API break

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unless I'm misreading this and this is just recording the status for audit

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's audit only. We don't have a proper way to express the hijack state. So neither the new nor the old audit event was correct. No need to revert IMO.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that's fine, I misread

return f.ResponseWriter.(http.Hijacker).Hijack()
}

Expand Down