Skip to content

HDDS-8829. Symmetric Keys for Delegation Tokens #7394

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

Merged
adoroszlai merged 6 commits into from
Nov 17, 2024
Merged

HDDS-8829. Symmetric Keys for Delegation Tokens #7394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9a6093b
HDDS-8829. Symmetric Keys for Delegation Tokens
ChenSammi Nov 5, 2024
74a8432
fix checkstyle
ChenSammi Nov 5, 2024
3649530
fix failed UT
ChenSammi Nov 5, 2024
1afacb0
add OM layout feature DELEGATION_TOKEN_SYMMETRIC_SIGN
ChenSammi Nov 6, 2024
e96c28a
refactor exsting code
ChenSammi Nov 12, 2024
32fa80e
add comments
ChenSammi Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/OzoneSecretManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,14 +172,13 @@ public int incrementDelegationTokenSeqNum() {
*/
private OzoneSecretKey updateCurrentKey(KeyPair keyPair,
X509Certificate certificate) {
logger.info("Updating current master key for generating tokens. Cert id {}",
certificate.getSerialNumber().toString());

int newCurrentId = incrementCurrentKeyId();
OzoneSecretKey newKey = new OzoneSecretKey(newCurrentId,
certificate.getNotAfter().getTime(), keyPair,
certificate.getSerialNumber().toString());
currentKey.set(newKey);
logger.info("Updated current master key for generating tokens. Cert id {}, Master key id {}",
certificate.getSerialNumber().toString(), newKey.getKeyId());
return newKey;
}

Expand Down
3 changes: 1 addition & 2 deletions ...server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,6 @@ public void stop() {
}

public static boolean isSecretKeyEnable(SecurityConfig conf) {
return conf.isSecurityEnabled() &&
(conf.isBlockTokenEnabled() || conf.isContainerTokenEnabled());
return conf.isSecurityEnabled();
Copy link
Contributor

@jojochuang jojochuang Nov 8, 2024

Choose a reason for hiding this comment

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

why is this changed?

Copy link
Contributor Author

@ChenSammi ChenSammi Nov 12, 2024

Choose a reason for hiding this comment

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

Before this patch, secretKey service only starts when block token or container token is enabled. With this patch, secretKey should start only when security is enabled, for delegation token is by default enabled when security is enabled.

}
}
100 changes: 59 additions & 41 deletions hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/security/OzoneTokenIdentifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,14 @@
import java.io.IOException;
import java.time.Instant;
import java.util.Arrays;
import java.util.UUID;

import com.google.common.base.Preconditions;
import org.apache.commons.lang3.StringUtils;
import org.apache.commons.lang3.builder.EqualsBuilder;
import org.apache.hadoop.hdds.annotation.InterfaceAudience;
import org.apache.hadoop.hdds.annotation.InterfaceStability;
import org.apache.hadoop.io.DataInputBuffer;
import org.apache.hadoop.io.DataOutputBuffer;
import org.apache.hadoop.io.Text;
import org.apache.hadoop.io.WritableUtils;
import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMTokenProto;
Expand All @@ -47,7 +49,11 @@ public class OzoneTokenIdentifier extends
AbstractDelegationTokenIdentifier {

public static final Text KIND_NAME = new Text("OzoneToken");
@Deprecated
// the certificate id of this OM, deprecated since HDDS-8829
private String omCertSerialId;
// shared secret key id generated by SCM.
private String secretKeyId;
private Type tokenType;
private String awsAccessId;
private String signature;
Expand Down Expand Up @@ -82,31 +88,6 @@ public Text getKind() {
return KIND_NAME;
}

/** Instead of relying on proto serialization, this
* provides explicit serialization for OzoneTokenIdentifier.
* @return byte[]
*/
public byte[] toUniqueSerializedKey() {
DataOutputBuffer buf = new DataOutputBuffer();
try {
super.write(buf);
WritableUtils.writeVInt(buf, getTokenType().getNumber());
// Set s3 specific fields.
if (getTokenType().equals(S3AUTHINFO)) {
WritableUtils.writeString(buf, getAwsAccessId());
WritableUtils.writeString(buf, getSignature());
WritableUtils.writeString(buf, getStrToSign());
} else {
WritableUtils.writeString(buf, getOmCertSerialId());
WritableUtils.writeString(buf, getOmServiceId());
}
} catch (java.io.IOException e) {
throw new IllegalArgumentException(
"Can't encode the the raw data ", e);
}
return buf.getData();
}

/** Instead of relying on proto deserialization, this
* provides explicit deserialization for OzoneTokenIdentifier.
* @return byte[]
Expand All @@ -125,20 +106,19 @@ public OzoneTokenIdentifier fromUniqueSerializedKey(byte[] rawData)
setStrToSign(WritableUtils.readString(in));
} else {
this.tokenType = Type.DELEGATION_TOKEN;
setOmCertSerialId(WritableUtils.readString(in));
String value = WritableUtils.readString(in);
try {
UUID.fromString(value);
setSecretKeyId(value);
} catch (IllegalArgumentException e) {
setOmCertSerialId(value);
}
Comment on lines +110 to +115
Copy link
Contributor

@jojochuang jojochuang Nov 8, 2024

Choose a reason for hiding this comment

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

using exception handling for this case is probably going to be expensive.
what about:

Suggested change
try {
UUID.fromString(value);
setSecretKeyId(value);
} catch (IllegalArgumentException e) {
setOmCertSerialId(value);
}
UUID.fromString(value);
if (this.omCertSerialId == null || this.secretKeyId == null) {
setSecretKeyId(value);
} else {
setOmCertSerialId(value);
}

Copy link
Contributor

@jojochuang jojochuang Nov 8, 2024

Choose a reason for hiding this comment

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

Oh wait this is not right. Why does it call UUID.fromString() and ignore the return value?

Copy link
Contributor Author

@ChenSammi ChenSammi Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Because this OzoneTokenIdentifier#toUniqueSerializedKey uses its own raw format instead of OMTokenProto format, which I have no idea why. In a period of time(7d, max delegation time lifetime) after OM is upgraded to use shared secret key, the same index string would be cerial ID or secreKey ID. So this " UUID.fromString(value);" used to detect whether it's secretKey, which is a UUID, or a serial ID, which is a number.

Let me further check if we can refactor some existing code, to avoid this UUID.fromString(value) being executed after upgrade 7days.

Copy link
Contributor

@jojochuang jojochuang Nov 12, 2024

Choose a reason for hiding this comment

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

I was going to suggest to use Integer.isNumeric(). But it throws an exception internally so it ends up the same.

setOmServiceId(WritableUtils.readString(in));
}
return this;
}

/**
* Overrides default implementation to write using Protobuf.
*
* @param out output stream
* @throws IOException
*/
@Override
public void write(DataOutput out) throws IOException {
public OMTokenProto toProtoBuf() throws IOException {
OMTokenProto.Builder builder = OMTokenProto.newBuilder()
.setMaxDate(getMaxDate())
.setType(getTokenType())
Expand All @@ -155,14 +135,28 @@ public void write(DataOutput out) throws IOException {
.setSignature(getSignature())
.setStrToSign(getStrToSign());
} else {
builder.setOmCertSerialId(getOmCertSerialId());
if (StringUtils.isNotEmpty(getOmCertSerialId())) {
builder.setOmCertSerialId(getOmCertSerialId());
}
if (StringUtils.isNotEmpty(getSecretKeyId())) {
builder.setSecretKeyId(getSecretKeyId());
}
if (getOmServiceId() != null) {
builder.setOmServiceId(getOmServiceId());
}
}
return builder.build();
}

OMTokenProto token = builder.build();
out.write(token.toByteArray());
/**
* Overrides default implementation to write using Protobuf.
*
* @param out output stream
* @throws IOException
*/
@Override
public void write(DataOutput out) throws IOException {
out.write(toProtoBuf().toByteArray());
}

/**
Expand All @@ -183,7 +177,12 @@ public void readFields(DataInput in) throws IOException {
setMaxDate(token.getMaxDate());
setSequenceNumber(token.getSequenceNumber());
setMasterKeyId(token.getMasterKeyId());
setOmCertSerialId(token.getOmCertSerialId());
if (token.hasOmCertSerialId()) {
setOmCertSerialId(token.getOmCertSerialId());
}
if (token.hasSecretKeyId()) {
setSecretKeyId(token.getSecretKeyId());
}

// Set s3 specific fields.
if (getTokenType().equals(S3AUTHINFO)) {
Expand Down Expand Up @@ -221,7 +220,12 @@ public static OzoneTokenIdentifier readProtoBuf(DataInput in)
identifier.setSequenceNumber(token.getSequenceNumber());
identifier.setMasterKeyId(token.getMasterKeyId());
}
identifier.setOmCertSerialId(token.getOmCertSerialId());
if (token.hasOmCertSerialId()) {
identifier.setOmCertSerialId(token.getOmCertSerialId());
}
if (token.hasSecretKeyId()) {
identifier.setSecretKeyId(token.getSecretKeyId());
}
identifier.setOmServiceId(token.getOmServiceId());
return identifier;
}
Expand Down Expand Up @@ -264,6 +268,7 @@ public boolean equals(Object obj) {
}
OzoneTokenIdentifier that = (OzoneTokenIdentifier) obj;
return new EqualsBuilder()
.append(getSecretKeyId(), that.getSecretKeyId())
.append(getOmCertSerialId(), that.getOmCertSerialId())
.append(getMaxDate(), that.getMaxDate())
.append(getIssueDate(), that.getIssueDate())
Expand Down Expand Up @@ -326,6 +331,18 @@ public String getOmCertSerialId() {

public void setOmCertSerialId(String omCertSerialId) {
this.omCertSerialId = omCertSerialId;
Preconditions.checkArgument(this.omCertSerialId == null || this.secretKeyId == null,
Copy link
Contributor

@duongkame duongkame Nov 8, 2024

Choose a reason for hiding this comment

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

nit: this would not validate it. Imagine you have omCertSerialId already set, then call setOmCertSerialId. The condition is passed because of the || operator.

Copy link
Contributor Author

@ChenSammi ChenSammi Nov 12, 2024

Choose a reason for hiding this comment

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

The check here is to make sure a OzoneTokenIdentifier cannot have both omCertSerialId and secretKeyId set. It can only have one of them.

"omCertSerialId and secretKeyId cannot both be valid");
}

public String getSecretKeyId() {
return secretKeyId;
}

public void setSecretKeyId(String id) {
this.secretKeyId = id;
Preconditions.checkArgument(this.omCertSerialId == null || this.secretKeyId == null,
"omCertSerialId and secretKeyId cannot both be valid");
}

public String getOmServiceId() {
Expand Down Expand Up @@ -383,7 +400,8 @@ public String toString() {
.append(", signature=").append(getSignature())
.append(", awsAccessKeyId=").append(getAwsAccessId())
.append(", omServiceId=").append(getOmServiceId())
.append(", omCertSerialId=").append(getOmCertSerialId());
.append(", omCertSerialId=").append(getOmCertSerialId())
.append(", secretKeyId=").append(getSecretKeyId());
return buffer.toString();
}
}
20 changes: 4 additions & 16 deletions ...op-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestSecretKeysApi.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import org.apache.hadoop.hdds.protocol.SecretKeyProtocol;
import org.apache.hadoop.hdds.scm.server.SCMHTTPServerConfig;
import org.apache.hadoop.hdds.scm.server.StorageContainerManager;
import org.apache.hadoop.hdds.security.exception.SCMSecretKeyException;
import org.apache.hadoop.hdds.security.symmetric.ManagedSecretKey;
import org.apache.hadoop.ipc.RemoteException;
import org.apache.hadoop.minikdc.MiniKdc;
Expand Down Expand Up @@ -67,7 +66,6 @@
import static org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_CLIENT_ADDRESS_KEY;
import static org.apache.hadoop.hdds.scm.server.SCMHTTPServerConfig.ConfigStrings.HDDS_SCM_HTTP_KERBEROS_KEYTAB_FILE_KEY;
import static org.apache.hadoop.hdds.scm.server.SCMHTTPServerConfig.ConfigStrings.HDDS_SCM_HTTP_KERBEROS_PRINCIPAL_KEY;
import static org.apache.hadoop.hdds.security.exception.SCMSecretKeyException.ErrorCode.SECRET_KEY_NOT_ENABLED;
import static org.apache.hadoop.hdds.utils.HddsServerUtil.getSecretKeyClientForDatanode;
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ADMINISTRATORS;
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_SECURITY_ENABLED_KEY;
Expand Down Expand Up @@ -245,24 +243,14 @@ public void testSecretKeyApiSuccess() throws Exception {
}

/**
* Verify API behavior when block token is not enable.
* Verify API behavior.
*/
@Test
public void testSecretKeyApiNotEnabled() throws Exception {
public void testSecretKeyApi() throws Exception {
startCluster(1);
SecretKeyProtocol secretKeyProtocol = getSecretKeyProtocol();

SCMSecretKeyException ex = assertThrows(SCMSecretKeyException.class,
secretKeyProtocol::getCurrentSecretKey);
assertEquals(SECRET_KEY_NOT_ENABLED, ex.getErrorCode());

ex = assertThrows(SCMSecretKeyException.class,
() -> secretKeyProtocol.getSecretKey(UUID.randomUUID()));
assertEquals(SECRET_KEY_NOT_ENABLED, ex.getErrorCode());

ex = assertThrows(SCMSecretKeyException.class,
secretKeyProtocol::getAllSecretKeys);
assertEquals(SECRET_KEY_NOT_ENABLED, ex.getErrorCode());
assertNull(secretKeyProtocol.getSecretKey(UUID.randomUUID()));
assertEquals(1, secretKeyProtocol.getAllSecretKeys().size());
}

/**
Expand Down
32 changes: 15 additions & 17 deletions ...-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestSecureOzoneCluster.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
import org.apache.hadoop.hdds.scm.server.StorageContainerManager;
import org.apache.hadoop.hdds.security.exception.SCMSecurityException;
import org.apache.hadoop.hdds.security.SecurityConfig;
import org.apache.hadoop.hdds.security.symmetric.ManagedSecretKey;
import org.apache.hadoop.hdds.security.symmetric.SecretKeyManager;
import org.apache.hadoop.hdds.security.x509.certificate.authority.CAType;
import org.apache.hadoop.hdds.security.x509.certificate.authority.DefaultApprover;
Expand Down Expand Up @@ -100,6 +101,7 @@
import org.apache.hadoop.ozone.om.protocolPB.OzoneManagerProtocolClientSideTranslatorPB;
import org.apache.hadoop.ozone.security.OMCertificateClient;
import org.apache.hadoop.ozone.security.OzoneTokenIdentifier;
import org.apache.hadoop.ozone.security.SecretKeyTestClient;
import org.apache.hadoop.security.KerberosAuthException;
import org.apache.hadoop.security.SaslRpcServer.AuthMethod;
import org.apache.hadoop.security.SecurityUtil;
Expand Down Expand Up @@ -152,7 +154,6 @@
import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.USER_MISMATCH;
import static org.apache.hadoop.security.UserGroupInformation.AuthenticationMethod.KERBEROS;

import org.apache.ozone.test.LambdaTestUtils;
import org.apache.ozone.test.tag.Flaky;
import org.apache.ozone.test.tag.Unhealthy;
import org.apache.ratis.protocol.ClientId;
Expand Down Expand Up @@ -1182,10 +1183,10 @@ public String renewAndStoreKeyAndCertificate(boolean force) throws CertificateEx
}

/**
* Tests delegation token renewal after a certificate renew.
* Tests delegation token renewal after a secret key rotation.
*/
@Test
void testDelegationTokenRenewCrossCertificateRenew() throws Exception {
void testDelegationTokenRenewCrossSecretKeyRotation() throws Exception {
initSCM();
try {
scm = HddsTestUtils.getScmSimple(conf);
Expand All @@ -1206,11 +1207,12 @@ void testDelegationTokenRenewCrossCertificateRenew() throws Exception {

CertificateClientTestImpl certClient =
new CertificateClientTestImpl(newConf, true);
X509Certificate omCert = certClient.getCertificate();
String omCertId1 = omCert.getSerialNumber().toString();
// Start OM
om.setCertClient(certClient);
om.setScmTopologyClient(new ScmTopologyClient(scmBlockClient));
SecretKeyTestClient secretKeyClient = new SecretKeyTestClient();
ManagedSecretKey secretKey1 = secretKeyClient.getCurrentSecretKey();
om.setSecretKeyClient(secretKeyClient);
om.start();
GenericTestUtils.waitFor(() -> om.isLeaderReady(), 100, 10000);

Expand All @@ -1231,30 +1233,26 @@ void testDelegationTokenRenewCrossCertificateRenew() throws Exception {
assertEquals(SecurityUtil.buildTokenService(
om.getNodeDetails().getRpcAddress()).toString(),
token1.getService().toString());
assertEquals(omCertId1, token1.decodeIdentifier().getOmCertSerialId());
assertEquals(secretKey1.getId().toString(), token1.decodeIdentifier().getSecretKeyId());

// Renew delegation token
long expiryTime = omClient.renewDelegationToken(token1);
assertThat(expiryTime).isGreaterThan(0);

// Wait for OM certificate to renew
LambdaTestUtils.await(certLifetime, 100, () ->
!StringUtils.equals(token1.decodeIdentifier().getOmCertSerialId(),
omClient.getDelegationToken(new Text("om"))
.decodeIdentifier().getOmCertSerialId()));
String omCertId2 =
certClient.getCertificate().getSerialNumber().toString();
assertNotEquals(omCertId1, omCertId2);
// Rotate secret key
secretKeyClient.rotate();
ManagedSecretKey secretKey2 = secretKeyClient.getCurrentSecretKey();
assertNotEquals(secretKey1.getId(), secretKey2.getId());
// Get a new delegation token
Token<OzoneTokenIdentifier> token2 = omClient.getDelegationToken(
new Text("om"));
assertEquals(omCertId2, token2.decodeIdentifier().getOmCertSerialId());
assertEquals(secretKey2.getId().toString(), token2.decodeIdentifier().getSecretKeyId());

// Because old certificate is still valid, so renew old token will succeed
// Because old secret key is still valid, so renew old token will succeed
expiryTime = omClient.renewDelegationToken(token1);
assertThat(expiryTime)
.isGreaterThan(0)
.isLessThan(omCert.getNotAfter().getTime());
.isLessThan(secretKey2.getExpiryTime().toEpochMilli());
} finally {
if (scm != null) {
scm.stop();
Expand Down
2 changes: 2 additions & 0 deletions ...tion-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerListVolumesSecure.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
import org.apache.hadoop.hdds.security.x509.certificate.client.CertificateClientTestImpl;
import org.apache.hadoop.minikdc.MiniKdc;
import org.apache.hadoop.ozone.OzoneAcl;
import org.apache.hadoop.ozone.client.SecretKeyTestClient;
import org.apache.hadoop.ozone.om.exceptions.OMException;
import org.apache.hadoop.ozone.om.helpers.OmVolumeArgs;
import org.apache.hadoop.ozone.om.protocolPB.OmTransportFactory;
Expand Down Expand Up @@ -201,6 +202,7 @@ private void setupEnvironment(boolean aclEnabled,
om.setScmTopologyClient(new ScmTopologyClient(
new ScmBlockLocationTestingClient(null, null, 0)));
om.setCertClient(new CertificateClientTestImpl(conf));
om.setSecretKeyClient(new SecretKeyTestClient());
om.start();

// Get OM client
Expand Down
3 changes: 2 additions & 1 deletion hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1457,7 +1457,8 @@ message OMTokenProto {
optional string accessKeyId = 12;
optional string signature = 13;
optional string strToSign = 14;
optional string omServiceId = 15;
optional string omServiceId = 15 [deprecated = true];
optional string secretKeyId = 16;
Copy link
Contributor

@duongkame duongkame Nov 8, 2024

Choose a reason for hiding this comment

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

Deprecated omCertSerialId?

Copy link
Contributor Author

@ChenSammi ChenSammi Nov 12, 2024

Choose a reason for hiding this comment

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

Right

}

message SecretKeyProto {
Expand Down
Loading