Skip to content

HDDS-6525. Add audit log for S3Gateway#3252

Merged
adoroszlai merged 14 commits intoapache:masterfrom
symious:HDDS-6525
Apr 19, 2022
Merged

HDDS-6525. Add audit log for S3Gateway#3252
adoroszlai merged 14 commits intoapache:masterfrom
symious:HDDS-6525

Conversation

@symious
Copy link
Contributor

@symious symious commented Mar 29, 2022

What changes were proposed in this pull request?

Currently There is no audit log for S3gateway.

This ticket is to add audit log for S3gateway

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-6525

How was this patch tested?

unit test.

@symious
Copy link
Contributor Author

symious commented Mar 30, 2022

@adoroszlai @ferhui Could you help to review this PR?

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @symious for working on this.

@adoroszlai adoroszlai added s3 S3 Gateway enhancement New feature or request labels Apr 11, 2022
@symious
Copy link
Contributor Author

symious commented Apr 11, 2022

@adoroszlai Thank you for the detailed review.

Fixed with the comments except the default value one.
I was thinking to compare the value with defaultValue, but seems it's possible for users to use a value same as defaultValue.

@adoroszlai
Copy link
Contributor

Thanks @symious for updating the patch.

There is a compile error after conflict resolution:

Error:  /home/runner/work/ozone/ozone/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java:[114,20] cannot find symbol
  symbol:   variable browser
  location: class org.apache.hadoop.ozone.s3.endpoint.BucketEndpoint

@symious
Copy link
Contributor Author

symious commented Apr 11, 2022

Updated, please have a look.

@adoroszlai
Copy link
Contributor

Thanks @symious for updating the patch. LGTM. Can you please resolve the conflict?

@symious
Copy link
Contributor Author

symious commented Apr 14, 2022

@adoroszlai Conflict solved, please have a look.

@adoroszlai adoroszlai merged commit 3fd7dc6 into apache:master Apr 19, 2022
@adoroszlai
Copy link
Contributor

Thanks @symious for the contribution, @ferhui for the review.

@symious
Copy link
Contributor Author

symious commented Apr 19, 2022

@adoroszlai @ferhui Thanks for the review.

@symious symious deleted the HDDS-6525 branch April 19, 2022 15:04

if (clientIp == null || clientIp.isEmpty()) {
// extract from forward ips
String ipForwarded = httpServletRequest.getHeader("x-forwarded-for");
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the rational for picking x-real-ip first before x-forwarded-for
For the purpose of auditing it might make sense to include both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried to find some comparisions, but can not find any definite answers:

  1. X-Real-IP should probably be preferred over X-Forwarded-For in _extraClientIP directive? akka/akka-http#1670 (comment).

It seems that even though the leftmost value of X-Forwarded-For is generally the original client IP address, this is not universally so, and there exist modules for at least Apache and Nginx web servers to provide the correct resolution of the client IP address and setting it as the X-Real-IP value.

  1. https://onefeed.xyz/posts/x-forwarded-for-vs-x-real-ip.html

It denpends on how much you know the network between the client and the server, and how much you trust these headers.
However, since any proxy can modify/add these headers freely, there is no guarantee the IP is of a real client.

@Provider
@PreMatching
@Priority(ClientIpFilter.PRIORITY)
public class ClientIpFilter implements ContainerRequestFilter {
Copy link
Contributor

@kerneltime kerneltime Apr 19, 2022

Choose a reason for hiding this comment

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

I think for purposes of audit trail it makes sense to capture as many details as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this for adding both header's IP in audit log?
Currently the audit log has one field to record IP, if added multi IPs in one field, might incur some errors when users are operating on this field.



S3GAction s3GAction = S3GAction.GET_BUCKET;
Map<String, String> auditParams = S3Utils.genAuditParam(
Copy link
Contributor

Choose a reason for hiding this comment

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

For audit purposes, it makes sense to capture the entire query sent and doing it centrally once similar to how the client IP is calculated. It will help identify bad clients who are not setting the correct params.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I updated the generation of the audit params in a new ticket: #3325.

Could you help to check?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request s3 S3 Gateway

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants