-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Send cross cluster api key signature as header #135674
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
Send cross cluster api key signature as header #135674
Conversation
); | ||
} | ||
return decode(header); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need the encoded header for the signature so I moved this to CrossClusterAccessHeaders
.
.apply(commonClusterConfig) | ||
.setting("xpack.security.remote_cluster_client.ssl.enabled", "true") | ||
.setting("xpack.security.remote_cluster_client.ssl.certificate_authorities", "remote-cluster-ca.crt") | ||
.configFile("signing.crt", Resource.fromClasspath("signing/signing.crt")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds a signature config to the query cluster to make sure we don't send it when communicating with older fulfilling clusters.
|
||
assertCrossClusterSearchSuccessfulWithoutResult(); | ||
|
||
// TODO add test for certificate identity configured for API key but no signature provided (should 401) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the cross cluster api key with a cert identity is available these test will be implemented here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do this in a separate PR to keep scope manageable.
da8e485
to
1ad64cf
Compare
@@ -1 +1 @@ | |||
esql_plan_with_no_columns,9186000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New transport version needed for make sure we don't serialize the new thread context header for a fulfilling cluster that doesn't support it.
private static final String STATIC_TEST_CLUSTER_ALIAS = "static_test_cluster"; | ||
|
||
public void testSignWithPemKeyConfig() { | ||
public void testSignWithPemKeyConfig() throws GeneralSecurityException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the verifier to move the auth error message building out of the verifier. That way the auth error can be constructed in a way that we can easily separate between cert chain validation error and signature verification error.
assert authentication.isApiKey() : "initial authentication for cross cluster access must be by API key"; | ||
assert false == authentication.isRunAs() : "initial authentication for cross cluster access cannot be run-as"; | ||
|
||
// TODO ALWAYS check if used api key has a certificate identity and do this verification conditionally based on that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This TODO will be addressed in a follow up.
this(credentialsHeader, crossClusterAccessSubjectInfo, null); | ||
} | ||
|
||
private CrossClusterAccessHeaders( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This constructor is only used when we read the headers, the other one when constructing the headers.
components.securityContext(), | ||
this.authenticationService, | ||
this.crossClusterApiKeySignatureManager, | ||
crossClusterApiKeySignatureManager, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pass the full manager here and only the verifier above because the manager will issue a new signer per remote cluster while we only have a single verifier per cluster.
if (connection.getTransportVersion().supports(ADD_CROSS_CLUSTER_API_KEY_SIGNATURE)) { | ||
crossClusterAccessHeaders.writeToContext(threadContext, signer); | ||
} else { | ||
crossClusterAccessHeaders.writeToContext(threadContext, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing a null
signer is the same as "do not generate a signature", not very explicit but probably acceptable. Even if a signature is configured there is no use in providing it unless the fulfilling cluster can validate it. I considered failing here but this could actually happen in production while upgrading and we don't want to fail requests during a rolling upgrade for example.
} catch (GeneralSecurityException e) { | ||
logger.debug("failed certificate validation for Signature [" + signature + "]", e); | ||
throw new ElasticsearchSecurityException("Failed to verify signature from [{}]", signature.certificates()[0], e); | ||
// Make sure the provided certificate chain is trusted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change only removes the try-catch to let the caller handle different exceptions however they please.
} | ||
|
||
private String certificateToString(X509Certificate certificate) { | ||
public static String certificateToString(X509Certificate certificate) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is changed to be static and public to be used in error messages.
); | ||
} | ||
|
||
private X509Certificate[] getTestCertificates() throws CertificateException, IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mocking certs is hard and therefore I'm just reading a real one here.
1ad64cf
to
1173919
Compare
Pinging @elastic/es-security (Team:Security) |
Hi @jfreden, I've created a changelog YAML for you. |
|
||
public Signer signerForClusterAlias(String clusterAlias) { | ||
return new Signer(clusterAlias); | ||
return keyPairByClusterAlias.containsKey(clusterAlias) ? new Signer(clusterAlias) : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has changed to only return a signer if we have a signing config. This makes it a little easier to understand if there is a signing config or not in a caller.
public void writeToContext(final ThreadContext ctx, @Nullable CrossClusterApiKeySignatureManager.Signer signer) throws IOException { | ||
var encodedSubjectInfo = crossClusterAccessSubjectInfo.encode(); | ||
if (signer != null) { | ||
var signature = signer.sign(encodedSubjectInfo, credentialsHeader); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the signed payload include anything request-specific (timestamps, nonces, etc.)? Do we need to worry about replay protection here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Started an internal thread on this. In summary: replay protection is already handled by TLS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more of a general question for my understanding; do we need to worry about header size limits? Would the new certificate information we are adding have any possibility of breaching them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is all sent over the transport protocol we don't have any special restrictions on headers. I think a concern could be if it gets expensive to generate, serialize, parse or validate the headers.
|
||
final String subjectInfoHeader = ctx.getHeader(CROSS_CLUSTER_ACCESS_SUBJECT_INFO_HEADER_KEY); | ||
if (subjectInfoHeader == null) { | ||
throw new IllegalArgumentException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Wondering if SecurityException
would be more appropriate here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code actually just moved from the SubjectInfo
class because I needed the encoded string before decoding it to create the signature.
I think IllegalArgumentException
is appropriate here since we're missing the subject info argument in the API call and it appears to be the convention throughout the RCS implementation. I think SecurityException
could make sense too, since you're missing a required auth argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Will leave it up to you whether or not you also want Slobodan to look it over
} | ||
|
||
public void writeToContext(final ThreadContext ctx) throws IOException { | ||
public void writeToContext(final ThreadContext ctx, @Nullable CrossClusterApiKeySignatureManager.Signer signer) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you could overload the method here:
public void writeToContext(final ThreadContext ctx) throws IOException {
return writeToContext(ctx, null);
}
which will cut down the footprint of the various writeToContext(threadContext, null);
diffs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reviewing! I was actually considering that but eventually decided not to since in production code we always specify the signer (unless we're on a different transport version, but that code will eventually go away).
I don't think we should add overloaded methods only to reduce the footprint of the test code, since it can result in test specific code that doesn't reflect reality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I see! yeah, it's usually a code smell to update src/main
merely for the sake of src/test
var signature = crossClusterAccessHeaders.signature(); | ||
// Always validate a signature if provided | ||
if (signature != null) { | ||
verifySignature(signature, crossClusterAccessHeaders); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is a good place to verify signature. Is there a reason why we don't pull verification into ApiKeyService
?
What do you think if we extend ApiKeyCredentials
with signature and let ApiKeyService
verify API key credential, signature and caching?
Another advantage is that we would also have CrossClusterAccessAuthenticationService#authenticateHeaders
method be able to do an early verification.
Also, if I'm not mistaken, throwing authentication exception here would cause us to audit the same request twice: once successfully because API keys auth succeeded and second failure because we failed the signature verification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well actually, I haven't fully thought about caching bit. Intuitively I'd say we should cache successful auth and signature verification results. But will need to look into this closely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We talked about this offline, the idea is to follow this up when we do the certificate identity verification.
In summary we'll:
- Do the signature and certificate verification up front and extract the certificate identity from the request around here before anything else happens (so before regular auth)
- Add a new certificateIdentity field to ApiKeyCredentials here and populate it around here
- Only validate that the certificateIdentity against the field from that api key doc in the index later somewhere around here
In terms of cache we agreed that at least initially we won't do any caching for the signature verification since the signature would be unique per user (subject info) and risks being very large when there are a lot of concurrent users. We should revisit this decision in the follow up.
static final Set<String> ALLOWED_TRANSPORT_HEADERS = Stream.concat( | ||
Stream.of( | ||
CROSS_CLUSTER_ACCESS_CREDENTIALS_HEADER_KEY, | ||
CROSS_CLUSTER_ACCESS_SUBJECT_INFO_HEADER_KEY, | ||
CROSS_CLUSTER_ACCESS_SIGNATURE_HEADER_KEY, | ||
AuditUtil.AUDIT_REQUEST_ID, | ||
Task.TRACE_STATE | ||
), | ||
Task.HEADERS_TO_COPY.stream() | ||
).collect(Collectors.toUnmodifiableSet()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Alternatively to this would be using Sets.union()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, that's cleaner!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
328e56b
to
66791a0
Compare
We have to trust the cert we're signing with. Introduced in elastic#135674. Resolves: elastic#136368
This PR is a follow up to #134137 and #134893. It adds serialization and verification of the new header:
This PR does not use the certificate identity that was added in #134604 to verify that the identity in the passed leaf certificate belongs to the signed cross cluster API key by matching it against the API key certificate identity pattern. That will be done in a follow up PR to keep the scope of this PR manageable.