Skip to content

HDDS-10200. OM may terminate due to NPE in S3SecretValue proto conversion #6089

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
Merged
adoroszlai merged 5 commits into from
Jan 25, 2024
Merged

HDDS-10200. OM may terminate due to NPE in S3SecretValue proto conversion #6089

adoroszlai merged 5 commits into from
Jan 25, 2024

Conversation

@adoroszlai
Copy link
Contributor

@adoroszlai adoroszlai commented Jan 24, 2024

What changes were proposed in this pull request?

Revoked S3 secret is stored in the cache as S3SecretValue with null secret value. Since the object is mutable, other thread could access the same instance before revocation, and might encounter NPE while trying to convert to protobuf.

2024-01-24 09:49:09,063 [om1-OMDoubleBufferFlushThread] ERROR ratis.OzoneManagerDoubleBuffer (ExitUtils.java:terminate(133)) - Terminating with exit status 2: During flush to DB encountered error in OMDoubleBuffer flush thread om1-OMDoubleBufferFlushThread
org.apache.ratis.util.ExitUtils$ExitException: During flush to DB encountered error in OMDoubleBuffer flush thread om1-OMDoubleBufferFlushThread when handling OMRequest: cmdType: SetS3Secret
traceID: ""
success: true
status: OK
SetS3SecretResponse {
  accessId: "testUser1accessId1"
  secretKey: "testsecret"
}

	at org.apache.ratis.util.ExitUtils.terminate(ExitUtils.java:141)
	at org.apache.ratis.util.ExitUtils.terminate(ExitUtils.java:151)
	at org.apache.hadoop.ozone.om.ratis.OzoneManagerDoubleBuffer.terminate(OzoneManagerDoubleBuffer.java:559)
	at org.apache.hadoop.ozone.om.ratis.OzoneManagerDoubleBuffer.addToBatch(OzoneManagerDoubleBuffer.java:413)
	at org.apache.hadoop.ozone.om.ratis.OzoneManagerDoubleBuffer.flushBatch(OzoneManagerDoubleBuffer.java:345)
	at org.apache.hadoop.ozone.om.ratis.OzoneManagerDoubleBuffer.flushCurrentBuffer(OzoneManagerDoubleBuffer.java:319)
	at org.apache.hadoop.ozone.om.ratis.OzoneManagerDoubleBuffer.flushTransactions(OzoneManagerDoubleBuffer.java:286)
	at java.lang.Thread.run(Thread.java:750)
Caused by: java.lang.NullPointerException
	at org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos$S3Secret$Builder.setAwsSecret(OzoneManagerProtocolProtos.java)
	at org.apache.hadoop.ozone.om.helpers.S3SecretValue.getProtobuf(S3SecretValue.java:100)
	at org.apache.hadoop.hdds.utils.db.DelegatedCodec.toCodecBuffer(DelegatedCodec.java:85)
	at org.apache.hadoop.hdds.utils.db.Codec.toDirectCodecBuffer(Codec.java:73)
	at org.apache.hadoop.hdds.utils.db.TypedTable.putWithBatch(TypedTable.java:171)
	at org.apache.hadoop.ozone.om.OmMetadataManagerImpl$1.addWithBatch(OmMetadataManagerImpl.java:1949)
	at org.apache.hadoop.ozone.om.response.s3.security.OMSetSecretResponse.addToDBBatch(OMSetSecretResponse.java:80)
	at org.apache.hadoop.ozone.om.response.OMClientResponse.checkAndUpdateDB(OMClientResponse.java:66)
	at org.apache.hadoop.ozone.om.ratis.OzoneManagerDoubleBuffer.lambda$9(OzoneManagerDoubleBuffer.java:407)
	at org.apache.hadoop.ozone.om.ratis.OzoneManagerDoubleBuffer.addToBatchWithTrace(OzoneManagerDoubleBuffer.java:244)
	at org.apache.hadoop.ozone.om.ratis.OzoneManagerDoubleBuffer.addToBatch(OzoneManagerDoubleBuffer.java:406)
	... 4 more

was seen in TestMultiTenantVolume (4 / 1000 runs).

This PR changes S3SecretValue to be immutable.

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

How was this patch tested?

TestMultiTenantVolume passed 10x100:
https://github.com/adoroszlai/ozone/actions/runs/7645630287

Regular CI:
https://github.com/adoroszlai/ozone/actions/runs/7645622076

@adoroszlai adoroszlai self-assigned this Jan 24, 2024
@adoroszlai adoroszlai requested review from myskov and szetszwo January 24, 2024 21:24
@adoroszlai
Copy link
Contributor Author

adoroszlai commented Jan 25, 2024

@ArafatKhan2198 can you please also take a look?

myskov
myskov approved these changes Jan 25, 2024
View reviewed changes
Copy link
Contributor

@myskov myskov left a comment

Choose a reason for hiding this comment

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

LGTM

sadanand48
sadanand48 approved these changes Jan 25, 2024
View reviewed changes
Copy link
Contributor

@sadanand48 sadanand48 left a comment

Choose a reason for hiding this comment

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

Thanks @adoroszlai the patch, LGTM

szetszwo
szetszwo reviewed Jan 25, 2024
View reviewed changes
Copy link
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

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

@adoroszlai , thanks for working on this! Just a few minor comments inlined.

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/S3SecretValue.java
private boolean isDeleted;
private long transactionLogIndex;
private final String kerberosID;
private final String awsSecret;
Copy link
Contributor

@szetszwo szetszwo Jan 25, 2024

Choose a reason for hiding this comment

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

An unrelated comment: It is not a good idea store a secret in a plaintext String and write it to db.

The usual practice is to encrypt it in byte[] and erase it after use.

szetszwo
szetszwo approved these changes Jan 25, 2024
View reviewed changes
Copy link
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

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

+1 the change looks good.

@adoroszlai adoroszlai merged commit 9238be3 into apache:master Jan 25, 2024
@adoroszlai
Copy link
Contributor Author

adoroszlai commented Jan 25, 2024

Thanks @myskov, @sadanand48, @szetszwo for the review.

@adoroszlai adoroszlai deleted the HDDS-10200 branch January 25, 2024 18:19
adoroszlai added a commit to adoroszlai/ozone that referenced this pull request Feb 21, 2024
@adoroszlai
HDDS-10200. OM may terminate due to NPE in S3SecretValue proto conv…
4dfaf32 
…ersion (apache#6089)

(cherry picked from commit 9238be3)
adoroszlai added a commit to adoroszlai/ozone that referenced this pull request Feb 21, 2024
@adoroszlai
HDDS-10200. OM may terminate due to NPE in S3SecretValue proto conv…
f3ca694 
…ersion (apache#6089)

(cherry picked from commit 9238be3)
@adoroszlai adoroszlai mentioned this pull request Feb 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@szetszwo szetszwo szetszwo approved these changes

@myskov myskov myskov approved these changes

@sadanand48 sadanand48 sadanand48 approved these changes

@ChenSammi ChenSammi Awaiting requested review from ChenSammi

Assignees

@adoroszlai adoroszlai

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@adoroszlai @szetszwo @myskov @sadanand48