Add delegation feature to PKI realm

[albertzaharovits]

Introduces the allow_delegation PKI Realm setting and the X509AuthenticationToken.isDelegated property and puts them both at work inside TransportDelegatePkiAction to implement the exchange of a certificate chain for an ES access token, aka PKI delegation.

If allow_delegation is true (default false) on a PKI realm then it will authenticate X509AuthenticationTokens created by the TransportDelegatePkiAction which have the delegated property set.

It is a follow-up and hinges on #43932
Relates #34396

[elasticmachine]

Pinging @elastic/es-security

[bizybot]

I am yet to finish my review, I am posting the comments where I think we need to discuss so
the conversation keeps going. Thank you.

[tvernum]

I left a few comments.

[tvernum]

I don't mind, but I would find this more readable as:

       for (int i = 1; i < chain.length; i++) {
            X509Certificate cert = chain[i-1];
            X509Certificate issuer = chain[i];
            if (false == cert.getIssuerX500Principal().equals(issuer.getSubjectX500Principal()) {
                return false;
            }
        }

WDYT?

[albertzaharovits]

LGTM, thanks for the suggestion!

[tvernum]

I feel as though this action (& the request/response) would be clearer with some variation on Authenticate in their name.

• TransportDelegatedPkiAuthenticateAction
• TransportDelegatePkiAuthenticationAction
• TransportAuthenticateDelegatedPkiAction
• ?

[albertzaharovits]

TransportDelegatePkiAuthenticationAction sounds best to me. I'll go with that.

[tvernum]

Can we get rid of the duplication of this code (here and the action)?
CertParsingUtils would be an OK place for an isCertificateChainOrdered method.

[albertzaharovits]

Thanks for suggesting CertParsingUtils !

[tvernum]

I don't follow what's going on here with the feature branch...

The development is being done directly on the main repo, and then we're proposing to merge it to master. That doesn't seem right.

I expected to see development on private branches, with PRs to merge into the feature branch on the main repo.
I don't think this should be merged to master. It introduces an incomplete feature that we don't want to ship as-is (because there aren't enough protections around the delegation), so we should hold off merging it until it's in a shippable form.

[bizybot]

I don't follow what's going on here with the feature branch...

The development is being done directly on the main repo, and then we're proposing to merge it to master. That doesn't seem right.

I expected to see development on private branches, with PRs to merge into the feature branch on the main repo.
I don't think this should be merged to master. It introduces an incomplete feature that we don't want to ship as-is (because there aren't enough protections around the delegation), so we should hold off merging it until it's in a shippable form.

Thank you, Tim,
I created the branch but forgot to convey it to Albert, I think we can use https://github.com/elastic/elasticsearch/tree/proxied-pki branch to merge this change. The branch will need to be updated as it was created a while ago.

[albertzaharovits]

@tvernum @bizybot I have addressed your reviews, please take another look :)

I don't follow what's going on here with the feature branch...

There is a story:

The plan was to divide #43796 into several pieces. In order to do that, and have PRs based on one another (chained), PRs must be branches in the origin repo. Because the first piece #43932 was a bug fix, it was based on master, and this PR was based on that. When the bug fix had been merged, I should've picked Yogesh's branch, merge master, and re-target this PR to that; Instead I pointed this one to master by oversight, instead of creating a new branch. I have retargeted this to proxy-pki.

[tvernum]

LGTM

[bizybot]

Thank you for addressing the comments. It looks good, though we could add some tests (with different cert chains) that can be taken up later since we are working against the branch.

[albertzaharovits]

@bizybot I'll add a test with a proper chain in a follow-up.

[bizybot]

LGTM, Thank you, Albert, for the following up on comments.
We can add the tests for the cert chain checks in the next PR.