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

Add option to store IP-Addresses and/or user-agents details in audit logs #19725

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

tpoxa
Copy link
Contributor

@tpoxa tpoxa commented Dec 18, 2023

This PR adds the option to log IP addresses and user agents in audit logs.
This new functionality is optional and can be enabled/disabled by the admin in the configuration.

related to goharbor/community#10

Issue being fixed

closes #18675, #16423, #5561

Please indicate you've done the following:

  • Well Written Title and Summary of the PR
  • Label the PR as needed. release-note/new-feature
  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Made sure tests are passing and test coverage is added if needed.
  • Considered the docs impact and opened a new docs issue or PR with docs changes if needed in website repository.

Screenshots:
Screenshot 2024-01-08 at 19 44 27
Screenshot 2024-01-08 at 19 45 16

@tpoxa tpoxa requested a review from a team as a code owner December 18, 2023 08:58
@tpoxa tpoxa force-pushed the add-client-ip-to-audit-logs branch 6 times, most recently from b724948 to ab6a92c Compare December 19, 2023 22:55
Copy link

codecov bot commented Dec 19, 2023

Codecov Report

Attention: Patch coverage is 52.71318% with 61 lines in your changes are missing coverage. Please review.

Project coverage is 67.46%. Comparing base (b7b8847) to head (d849179).
Report is 117 commits behind head on main.

❗ Current head d849179 differs from pull request most recent head eb5fc28. Consider uploading reports for the commit eb5fc28 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #19725      +/-   ##
==========================================
- Coverage   67.56%   67.46%   -0.10%     
==========================================
  Files         991      997       +6     
  Lines      109181   109959     +778     
  Branches     2719     2724       +5     
==========================================
+ Hits        73768    74185     +417     
- Misses      31449    31789     +340     
- Partials     3964     3985      +21     
Flag Coverage Δ
unittests 67.46% <52.71%> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/controller/systeminfo/controller.go 68.35% <100.00%> (+1.68%) ⬆️
src/lib/config/config.go 86.04% <100.00%> (ø)
src/lib/config/userconfig.go 67.51% <100.00%> (-0.46%) ⬇️
src/lib/context.go 100.00% <100.00%> (ø)
src/pkg/audit/model/model.go 100.00% <ø> (ø)
...portal/src/app/base/left-side-nav/config/config.ts 100.00% <100.00%> (ø)
...ide-nav/config/system/system-settings.component.ts 54.54% <ø> (ø)
src/portal/src/app/services/app-config.ts 100.00% <ø> (ø)
src/core/middlewares/middlewares.go 0.00% <0.00%> (ø)
...app/base/left-side-nav/log/recent-log.component.ts 60.46% <50.00%> (-3.43%) ⬇️
... and 7 more

... and 22 files with indirect coverage changes

@NitroCao
Copy link

NitroCao commented Jan 5, 2024

Strongly hope that this PR will be merged into 2.11.0!! This PR will close #18675, #16423, #5561 and goharbor/community#10

@tpoxa tpoxa force-pushed the add-client-ip-to-audit-logs branch from cc0498e to 8bb1f82 Compare January 7, 2024 18:24
@Vad1mo Vad1mo changed the title add option to store ip addresses and/or user-agents in audit logs Add option to store IP-Addresses and/or user-agents details in audit logs Jan 8, 2024
@Vad1mo Vad1mo added the release-note/new-feature New Harbor Feature label Jan 8, 2024
@Vad1mo Vad1mo enabled auto-merge (squash) January 9, 2024 10:27
src/portal/src/i18n/lang/en-us-lang.json Show resolved Hide resolved
src/portal/src/i18n/lang/en-us-lang.json Show resolved Hide resolved
src/portal/src/i18n/lang/en-us-lang.json Outdated Show resolved Hide resolved
src/portal/src/i18n/lang/de-de-lang.json Outdated Show resolved Hide resolved
src/portal/src/i18n/lang/de-de-lang.json Outdated Show resolved Hide resolved
src/portal/src/i18n/lang/de-de-lang.json Outdated Show resolved Hide resolved
src/portal/src/i18n/lang/de-de-lang.json Outdated Show resolved Hide resolved
src/portal/src/i18n/lang/de-de-lang.json Outdated Show resolved Hide resolved
auto-merge was automatically disabled January 12, 2024 20:15

Head branch was pushed to by a user without write access

@tpoxa tpoxa force-pushed the add-client-ip-to-audit-logs branch from fc6d5bd to d849179 Compare January 27, 2024 18:47
@Vad1mo Vad1mo enabled auto-merge (squash) February 6, 2024 17:57
auto-merge was automatically disabled February 10, 2024 14:21

Head branch was pushed to by a user without write access

@tpoxa tpoxa force-pushed the add-client-ip-to-audit-logs branch from e48fd1c to 86e7be0 Compare February 10, 2024 14:21
@Vad1mo Vad1mo enabled auto-merge (squash) February 21, 2024 10:43
auto-merge was automatically disabled February 25, 2024 19:38

Head branch was pushed to by a user without write access

@tpoxa tpoxa force-pushed the add-client-ip-to-audit-logs branch from 86e7be0 to e8455c7 Compare February 25, 2024 19:38
@tpoxa tpoxa force-pushed the add-client-ip-to-audit-logs branch from e8455c7 to 3b4d564 Compare March 2, 2024 16:13
Maksym Trofimenko added 2 commits March 24, 2024 22:10
Signed-off-by: Maksym Trofimenko <[email protected]>
update translations
Signed-off-by: Maksym Trofimenko <[email protected]>
@tpoxa tpoxa force-pushed the add-client-ip-to-audit-logs branch from 3b4d564 to eb5fc28 Compare March 24, 2024 22:10
@Ankurk99
Copy link

Hello Harbor team, we are looking forward for this feature. Could you please provide an update on the plans for it? Are there any blockers, or is it a work in progress?

@WAR-S
Copy link

WAR-S commented May 28, 2024

Hey harbor team, we need this feature))

Copy link

This PR is being marked stale due to a period of inactivty. If this PR is still relevant, please comment or remove the stale label. Otherwise, this PR will close in 30 days.

@reasonerjt
Copy link
Contributor

In terms of the client IP. I still have some concerns regarding the accuracy.

Have you tested when the client is from a different network or behind a VPN?

Copy link
Contributor

@reasonerjt reasonerjt left a comment

Choose a reason for hiding this comment

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

Setting it to Request changes to avoid it from being accidentally merged before we are aligned about the expectation and limitation.

@Vad1mo
Copy link
Member

Vad1mo commented Aug 27, 2024

In terms of the client IP. I still have some concerns regarding the accuracy.

Have you tested when the client is from a different network or behind a VPN?

@reasonerjt, this works as it should also be over VPN, It doesn't do any magic things.

It also works together with different Ingress controllers (ingress nginx, AWS ALB, istio etc.)

@kofj
Copy link
Contributor

kofj commented Sep 4, 2024

In my point, I agree we need record client ip to audit logs, but recorded it to database not a good idea. Maybe we need refactoring audit log system to storage mass data.

forRegex = regexp.MustCompile(`(?i)(?:for=)([^(;|,| )]+)`)
)

func getIP(r *http.Request) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

if client request through multi-level proxy server(e.g. client-->proxy 1-->proxy 2->harbor ), what's the function will return ?

Choose a reason for hiding this comment

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

The function returns output based on the HTTP headers present in the request:

  • XFF (X-Forwarded-For): This here will return the first IP (usually* the client’s IP).
  • X-Real-IP: This will return the IP as set by the last proxy. It could be either the client’s IP or the last proxy’s IP, depending on how the proxies are configured.
  • Forwarded: This here will capture the first "for=" entry, which is usually* the client’s IP.

I’ve used "usually" because these values can be easily altered by proxies in the middle. To prevent this, we should have a way to define which proxies we trust. This issue has already been raised here .

Hope that answers your question.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add client IP for audit logs