Skip to content

Conversation

@cfryanr
Copy link
Contributor

@cfryanr cfryanr commented Jan 6, 2025

Previously the Supervisor audit logs logged the http.Request's remoteAddr value for all incoming http requests. However, this does not propagate the values of the X-Forwarded-For or X-Real-Ip headers into the audit logs. This would lose some valuable information in the case where a proxy server is placed in front of the Supervisor.

This PR removes the remoteAddr field from the Supervisor's HTTP Request Received audit log events, and replaces it with a new sourceIPs field. The value of this field is filled using the same code that fills the sourceIPs field in the Kubernetes audit logs, so it will have the exact same behavior. The sourceIPs value will always be an array of strings, and each string will be an IP address. The request's remoteAddr will always be the last value in the list. Other values included in the list will be the IP addresses taken from the request's X-Forwarded-For and/or X-Real-Ip headers.

This is a breaking change for anyone who has built custom tooling to parse the Supervisor's audit logs. Please note the change in the field name, and note that the new sourceIPs field's value will always be an array of IP addresses instead of a single IP address. Since we just recently released the audit logging feature, hopefully this does not actually break anyone.

Here is a real example from a Supervisor log after running the following command:

curl \
  --cacert root_ca.crt  \
  "https://pinniped-supervisor-clusterip.supervisor.svc.cluster.local/some/path/.well-known/openid-configuration" \
  -H "X-Forwarded-For: 1.2.3.4, 5.6.7.8"

Log output includes the X-Forwarded-For values followed by the remoteAddr value:

{
  "level": "info",
  "timestamp": "2025-01-06T20:04:31.649240Z",
  "caller": "go.pinniped.dev/internal/federationdomain/requestlogger/request_logger.go:84$requestlogger.(*requestLogger).logRequestReceived",
  "message": "HTTP Request Received",
  "auditEvent": true,
  "auditID": "183bd147-0448-49dc-b042-1d6f706f4c76",
  "proto": "HTTP/2.0",
  "method": "GET",
  "host": "pinniped-supervisor-clusterip.supervisor.svc.cluster.local",
  "serverName": "pinniped-supervisor-clusterip.supervisor.svc.cluster.local",
  "path": "/some/path/.well-known/openid-configuration",
  "userAgent": "curl/8.7.1",
  "sourceIPs": [
    "1.2.3.4",
    "5.6.7.8",
    "10.244.0.17"
  ]
}

Release note:

The `remoteAddr` key in the Supervisor's `HTTP Request Received` audit log event has been removed
and replaced with a new key called `sourceIPs`. The value of `sourceIPs` is always an array of string
IP addresses, and the last item in the list is always the address that was previously shown as the
`remoteAddr`. Other items in the list can come from the `X-Forwarded-For` and `X-Real-Ip` request
headers. See `sourceIPs` in https://kubernetes.io/docs/reference/config-api/apiserver-audit.v1
for details.

@netlify
Copy link

netlify bot commented Jan 6, 2025

Deploy Preview for pinniped-dev canceled.

Name Link
🔨 Latest commit 9619a0f
🔍 Latest deploy log https://app.netlify.com/sites/pinniped-dev/deploys/677c9da03475f50008562db5

@joshuatcasey joshuatcasey enabled auto-merge January 6, 2025 20:00
@joshuatcasey
Copy link
Contributor

LGTM. I like that we're choosing to switch to the K8s audit log "standard" even if that is different than what we had before.

I would probably have chosen to unit test the sourceIPs function by itself just enough to prove that it called utilnet.SourceIPs and kept that detail out of the TestLogRequestReceived unit tests, but it doesn't really matter.

@codecov
Copy link

codecov bot commented Jan 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 31.96%. Comparing base (23f414c) to head (9619a0f).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2174   +/-   ##
=======================================
  Coverage   31.95%   31.96%           
=======================================
  Files         379      379           
  Lines       62098    62108   +10     
=======================================
+ Hits        19843    19852    +9     
- Misses      41724    41726    +2     
+ Partials      531      530    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@joshuatcasey joshuatcasey merged commit e4f7b5d into main Jan 7, 2025
39 checks passed
@joshuatcasey joshuatcasey deleted the audit_sourceips branch January 7, 2025 05:09
@cfryanr cfryanr mentioned this pull request Jan 14, 2025
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants