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.

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

Always include remoteAddr in source IP list for audit #87167

Open
wants to merge 2 commits into
base: master
from

Conversation

@tallclair
Copy link
Member

tallclair commented Jan 13, 2020

Since the remoteAddr is much harder to spoof than headers, always include it in
the list of source IPs used in audit logs.

This PR updates the logic for the source IP list to be:

  1. List of valid X-Forwarded-For IPs
  2. The X-Real-IP, if not already contained in the X-Forwarded-For list
  3. The reqest's RemoteAddr, if it's not already the last value in the list.

/kind bug

Does this PR introduce a user-facing change?:

The audit event sourceIPs list will now always end with the IP that sent the request directly to the API server.

/sig auth
/priority important-longterm
/area audit
/assign @mikedanese

Since the remoteAddr is much harder to spoof than headers, always include it in
the list of source IPs used in audit logs.
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 13, 2020

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tallclair
To complete the pull request process, please assign liggitt
You can assign the PR to them by writing /assign @liggitt in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot requested review from deads2k and sttts Jan 13, 2020
@tallclair

This comment has been minimized.

Copy link
Member Author

tallclair commented Jan 14, 2020

/retest

return []net.IP{ip}
// Only append the X-Real-Ip if it's not already contained in the X-Forwarded-For chain.
if ip != nil && !containsIP(srcIPs, ip) {
srcIPs = append(srcIPs, ip)

This comment has been minimized.

Copy link
@mikedanese

mikedanese Jan 14, 2020

Member

Should x-real-ip be prepended?

This comment has been minimized.

Copy link
@tallclair

tallclair Jan 15, 2020

Author Member

Probably, except that would change the logic of ClientIP. Maybe the solution is to just break the dependency of ClientIP on SourceIPs, what do you think?

Copy link
Member Author

tallclair left a comment

Thanks for the review!

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 15, 2020

@tallclair: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce e6821bd link /test pull-kubernetes-e2e-gce

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.