PKI Authentication Delegation in new endpoint #43796

albertzaharovits · 2019-06-30T19:02:45Z

This implementation creates new REST and transport actions (one of each) that do the validation of a certificate chain against installed PKI realms. It is expected that the client calling the endpoint performed TLS termination and captured the clients certificate. Upon successful validation an access token is released (but no refresh token). The access token (Bearer) impersonates the end-user as if the end-user, owner of the private key associated with the certificate chain, authenticated directly to ES.

It is draft because it does not contain tests.

This would be a rather prosaic implementation, if not for a bug fix, that I would describe in a separate PR.

Relates #34396

elasticmachine · 2019-06-30T19:02:46Z

Pinging @elastic/es-security

albertzaharovits · 2019-07-01T07:13:48Z

@elasticmachine run elasticsearch-ci/default-distro

albertzaharovits · 2019-07-01T21:22:19Z

Since I didn't yet managed to raise the promised bug fix PR, I will detail it here to cut the suspense.

The X509AuthenticationToken has a principal field which is set during the token construction. The field is set by parsing the subject DN of the client cert. Given the current inner workings, of first iterating the realms to build the token and afterwards iterating them again to authenticate the token, it was possible to parse the DN using the pattern of some PKI realm but actually authenticating the token by another PKI realm (given the truststore configurations).

The proposed fix (in this PR, but I will raise a separate PR just for the fix) is to simply consider the un-parsed DN as the principal of the X509AuthenticationToken. The principal after authentication is still the one after the parsing.

tvernum · 2019-07-02T04:49:23Z

Although line count wise this isn't huge, the weight of it feels big to me.
That is, because it attempts to solve the whole space in 1 PR (and merge direct to master/7.x), I know that I need to review it in terms of "is this a complete feature?" as well as "does every part of this make sense?".
To feel like I can do justice to the review, I think would need to do several passes, each one considering it from a different angle.

It would be my preference (despite the relatively small line count) to split this into smaller PRs if possible so we can review the various separate aspects independently - e.g. "Here is the REST endpoint, Request & Response objects, the transport action is stubbed out & doesn't do anything."

bizybot

I agree with Tim to split this into smaller changes, this is based on my observation from my earlier PRs and would help everyone to focus one thing at a time. Thank you.
As I had started to review this earlier, I have posted the comments here in case we move to split this PR.

bizybot · 2019-07-01T23:04:38Z

...ugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/DelegatePkiRequest.java

+    private X509Certificate[] certificates;
+
+    public DelegatePkiRequest(X509Certificate[] certificates) {
+        this.certificates = certificates;


null and empty check

bizybot · 2019-07-01T23:05:05Z

...ugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/DelegatePkiRequest.java

+import java.security.cert.X509Certificate;
+import java.util.Arrays;
+
+public class DelegatePkiRequest extends ActionRequest {


make it final?

I will make it final. But, in general, I think we should make final only the classes with algorithms inside, POJOs like requests and responses would generally benefit from inheritance.

bizybot · 2019-07-01T23:16:02Z

...ugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/DelegatePkiRequest.java

+    private X509Certificate[] certificates;
+
+    public DelegatePkiRequest(X509Certificate[] certificates) {
+        this.certificates = certificates;


I guess the expectation is that this will be ordered chain, shall we check that here? or try to order it before setting it?

They are already in the correct order (as the client provided them), and the order is checked by trustManager.checkClientTrusted . The client should dump the cert chain as captured from the TLS connection.

bizybot · 2019-07-01T23:16:43Z

...gin/core/src/main/java/org/elasticsearch/xpack/core/security/action/DelegatePkiResponse.java

+    DelegatePkiResponse() { }
+
+    public DelegatePkiResponse(String tokenString, TimeValue expiresIn) {
+        this.tokenString = Objects.requireNonNull(tokenString);


null or empty check

bizybot · 2019-07-01T23:17:11Z

...gin/core/src/main/java/org/elasticsearch/xpack/core/security/action/DelegatePkiResponse.java

+
+public class DelegatePkiResponse extends ActionResponse implements ToXContentObject {
+
+    private String tokenString;


make it final?

bizybot · 2019-07-01T23:17:19Z

...gin/core/src/main/java/org/elasticsearch/xpack/core/security/action/DelegatePkiResponse.java

+public class DelegatePkiResponse extends ActionResponse implements ToXContentObject {
+
+    private String tokenString;
+    private TimeValue expiresIn;


make it final?

bizybot · 2019-07-01T23:19:16Z

...ty/src/main/java/org/elasticsearch/xpack/security/authc/pki/AuthenticationDelegateeInfo.java

+import java.io.IOException;
+import java.util.Objects;
+
+public class AuthenticationDelegateeInfo implements ToXContentObject {


I am trying to understand the purpose of this class. It feels like a wrapper around the original Authentication and we add it to the user metadata. Do we intend to use this information somewhere after authentication? Could you please elaborate, also if you could add java docs. Thank you.

This class should encapsulate all the details based on which the realm allows the delegation or not. At its simplest it could be a boolean flag saying that this token is delegated (as opposed to locally crafted), but here it encapsulates the authentication of the client. In this case the realm could restrict delegation to only certain users. I will leave it as a boolean, before we decide on the means to restrict delegation.

bizybot · 2019-07-03T06:17:14Z

...ecurity/src/test/java/org/elasticsearch/xpack/security/authc/pki/PkiAuthenticationTests.java

@@ -101,7 +101,8 @@ public void testRestAuthenticationFailure() throws Exception {
            try (CloseableHttpResponse response = SocketAccess.doPrivileged(() -> client.execute(put))) {
                assertThat(response.getStatusLine().getStatusCode(), is(401));
                String body = EntityUtils.toString(response.getEntity());
-                assertThat(body, containsString("unable to authenticate user [Elasticsearch Test Client]"));
+                assertThat(body, containsString(


As we do not extract DN upfront anymore the REST response on failure has changed. Do we think of it as a breaking change from the REST endpoint or it is okay?

The message, although part of the body, is the reason field in the ElasticsearchException . I believe we've been pretty lax about changes to it, but I can't find a recent PR that changed them. But generally I see we put messages from other internal exceptions in there so changes can happen unknowingly. So, I think this is not a breaking change, but might benefit from @tvernum opinion as well.

bizybot · 2019-07-03T06:17:45Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/pki/PkiRealm.java

                } else if (logger.isDebugEnabled()) {
-                    logger.debug("failed certificate validation for principal [{}]", token.principal());
+                    logger.debug("failed certificate validation for Subbject DN [{}]", token.dn());


Suggested change

logger.debug("failed certificate validation for Subbject DN [{}]", token.dn());

logger.debug("failed certificate validation for Subject DN [{}]", token.dn());

albertzaharovits · 2019-07-03T21:16:06Z

Thanks @bizybot for the very fast review. I have acknowledged the feedback and I will address it in the ensuing followups as suggested.

albertzaharovits · 2019-07-18T16:16:24Z

After #44561 has been raised there is no more code here that hasn't been merged or is on a PR or feature branch (https://github.com/elastic/elasticsearch/tree/proxied-pki), so closing.

albertzaharovits added 7 commits June 29, 2019 10:50

WIP

a47a53b

Action WIP

3c3b3da

Merge branch 'master' into pki-delegation

d982d14

Transport request and response

685e3d1

Bulk

7410765

DelegatePkiRequest

6c13a12

Rest action

6095a20

albertzaharovits added >feature :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v8.0.0 v7.3.0 labels Jun 30, 2019

albertzaharovits requested review from bizybot and tvernum June 30, 2019 19:02

albertzaharovits self-assigned this Jun 30, 2019

albertzaharovits added 2 commits July 1, 2019 09:22

Checkstyle

53c35b3

Logger usage check saved the day

86caa78

albertzaharovits added 3 commits July 1, 2019 10:20

RestDelegatePkiAction not RestDeletePrivilegesAction

a86de5e

PkiAuthenticationTests

2180f1a

Checkstyle

9297562

bizybot reviewed Jul 3, 2019

View reviewed changes

jpountz added v7.4.0 and removed v7.3.0 labels Jul 3, 2019

albertzaharovits mentioned this pull request Jul 3, 2019

Fix X509AuthenticationToken principal #43932

Merged

albertzaharovits mentioned this pull request Jul 16, 2019

Add delegation feature to PKI realm #44106

Merged

albertzaharovits closed this Jul 18, 2019

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PKI Authentication Delegation in new endpoint #43796

PKI Authentication Delegation in new endpoint #43796

albertzaharovits commented Jun 30, 2019

elasticmachine commented Jun 30, 2019

albertzaharovits commented Jul 1, 2019

albertzaharovits commented Jul 1, 2019

tvernum commented Jul 2, 2019

bizybot left a comment •

edited

Loading

bizybot Jul 1, 2019

bizybot Jul 1, 2019

albertzaharovits Jul 3, 2019

bizybot Jul 1, 2019

albertzaharovits Jul 3, 2019

bizybot Jul 1, 2019

bizybot Jul 1, 2019

bizybot Jul 1, 2019

bizybot Jul 1, 2019

albertzaharovits Jul 3, 2019

bizybot Jul 3, 2019

albertzaharovits Jul 3, 2019

bizybot Jul 3, 2019

albertzaharovits commented Jul 3, 2019

albertzaharovits commented Jul 18, 2019


		public class DelegatePkiResponse extends ActionResponse implements ToXContentObject {

		private String tokenString;

	logger.debug("failed certificate validation for Subbject DN [{}]", token.dn());
	logger.debug("failed certificate validation for Subject DN [{}]", token.dn());

PKI Authentication Delegation in new endpoint #43796

PKI Authentication Delegation in new endpoint #43796

Conversation

albertzaharovits commented Jun 30, 2019

elasticmachine commented Jun 30, 2019

albertzaharovits commented Jul 1, 2019

albertzaharovits commented Jul 1, 2019

tvernum commented Jul 2, 2019

bizybot left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits commented Jul 3, 2019

albertzaharovits commented Jul 18, 2019

bizybot left a comment •

edited

Loading