Parse byte array of otherName SANs to string of ASN1 object and OID by jksmth · Pull Request #943 · opensearch-project/security

jksmth · 2020-12-30T10:24:08Z

Signed-off-by: Jake Smith jakemgsmith@gmail.com

opendistro-for-elasticsearch/security pull request intake form

Category: Bug fix
Github Issue SSL certificate hot reload with OtherName in SAN #843
Description of changes:
Checks for type 0 (otherName) Subject Alternate Names (SANs) when validating certificate DNs, if found the byte array is read and if is a valid ASN1 object the OID and value is decoded to provide a string representation of the otherName. This is achieved with the use of bouncycastle lib.
Why these changes are required?
Per SSL certificate hot reload with OtherName in SAN #843 cert.getSubjectAlternativeNames().toString() does not lookup type 0 (otherName) SANs correctly and returns a string representing the byte array which changes on each iteration. This causes hot reload to fail if the certificate includes an otherName in the SAN as the string of the SANs returned by cert.getSubjectAlternativeNames().toString() will never match the current and new certificate in the hasValidDNs() method.
What is the old behavior before changes and new behavior after changes?

Old behaviour
Getting currently loaded certificates with GET /_opendistro/_security/api/ssl/certs would return:

{
  "http_certificates_list" : [
    {
      "issuer_dn" : "CN=elastic-test-http,OU=elastic-test",
      "subject_dn" : "CN=elastic-test-es-http.local,OU=elastic-test",
      "san" : "[[2, elastic-test-es-http]]",
      "not_before" : "2020-11-19T12:42:57Z",
      "not_after" : "2020-11-19T12:57:57Z"
    }
  ],
  "transport_certificates_list" : [
    {
      "issuer_dn" : "CN=elastic-test-transport,OU=elastic-test",
      "subject_dn" : "CN=elastic-test-es-node-0.local,OU=elastic-test",
      "san" : "[[0, [B@6ac191ff], [2, elastic-test-es-node-0], [7, 127.0.0.1]]",
      "not_before" : "2020-11-19T11:48:35Z",
      "not_after" : "2020-11-19T12:03:35Z"
    }
  ]
}

Note [0, [B@6ac191ff] in transport certificate san. Repeated calls to GET /_opendistro/_security/api/ssl/certs shows that the string representation of the OtherName byte array (B@xxxxxxxx) changes each time.

New behaviour
Getting currently loaded certificates with GET /_opendistro/_security/api/ssl/certs will now return:

{
  "http_certificates_list" : [
    {
      "issuer_dn" : "CN=elastic-test-http,OU=elastic-test",
      "subject_dn" : "CN=elastic-test-es-http.local,OU=elastic-test",
      "san" : "[[2, elastic-test-es-http]]",
      "not_before" : "2020-11-19T12:42:57Z",
      "not_after" : "2020-11-19T12:57:57Z"
    }
  ],
  "transport_certificates_list" : [
    {
      "issuer_dn" : "CN=elastic-test-transport,OU=elastic-test",
      "subject_dn" : "CN=elastic-test-es-node-0.local,OU=elastic-test",
      "san" : "[[0, [2.5.4.3, elastic-test-es-node-0]], [2, elastic-test-es-node-0], [7, 127.0.0.1]]",
      "not_before" : "2020-11-19T11:48:35Z",
      "not_after" : "2020-11-19T12:03:35Z"
    }
  ]
}

Note [0, [2.5.4.3, elastic-test-es-node-0]] in transport certificate san is now decoded to a list toString() of [oid, value]

Testing done:
Verified by manually testing and unit tests. New test certificates need creating to include an otherName in the SAN.
TO-DOs, if any:
Is it backport from master branch?
No

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

jksmth · 2020-12-30T10:31:29Z

To update the unit tests, certificates and keystrores need updating in src/test/resources/ssl/reload to include otherName in the SAN, is there a script or openssl config file that is used for generating these?

codecov · 2020-12-30T11:08:52Z

Codecov Report

Merging #943 (40da1f7) into main (3f6efe9) will increase coverage by 0.11%.
The diff coverage is 66.66%.

@@             Coverage Diff              @@
##               main     #943      +/-   ##
============================================
+ Coverage     64.47%   64.59%   +0.11%     
+ Complexity     3224     3173      -51     
============================================
  Files           244      244              
  Lines         17160    17153       -7     
  Branches       3040     3042       +2     
============================================
+ Hits          11064    11080      +16     
+ Misses         4555     4527      -28     
- Partials       1541     1546       +5

Impacted Files	Coverage Δ	Complexity Δ
...curity/ssl/ExternalOpenDistroSecurityKeyStore.java	`48.00% <0.00%> (-0.98%)`	`7.00 <0.00> (ø)`
...ecurity/ssl/DefaultOpenDistroSecurityKeyStore.java	`68.93% <67.56%> (-0.18%)`	`74.00 <4.00> (+3.00)`	⬇️
...ssl/rest/OpenDistroSecuritySSLCertsInfoAction.java	`61.11% <100.00%> (+2.49%)`	`5.00 <0.00> (ø)`
...rch/security/configuration/DlsFlsRequestValve.java	`0.00% <0.00%> (-50.00%)`	`0.00% <0.00%> (ø%)`
...relasticsearch/security/auditlog/NullAuditLog.java	`0.00% <0.00%> (-33.34%)`	`0.00% <0.00%> (-7.00%)`
...asticsearch/security/support/ReflectionHelper.java	`40.38% <0.00%> (-7.21%)`	`6.00% <0.00%> (-19.00%)`
...oforelasticsearch/security/support/ModuleType.java	`89.09% <0.00%> (-3.64%)`	`12.00% <0.00%> (-1.00%)`
...urity/configuration/PrivilegesInterceptorImpl.java	`50.00% <0.00%> (-3.29%)`	`30.00% <0.00%> (-1.00%)`
...earch/security/privileges/PrivilegesEvaluator.java	`65.95% <0.00%> (-2.30%)`	`78.00% <0.00%> (-1.00%)`
...ration/OpenDistroSecurityIndexSearcherWrapper.java	`88.88% <0.00%> (-2.23%)`	`21.00% <0.00%> (-1.00%)`
... and 42 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f6efe9...40da1f7. Read the comment docs.

debjanibnrj

Thanks for your contribution @jksmth!

You're solution looks good, could you add unit tests to this as well and submit as a PR.

...va/com/amazon/opendistroforelasticsearch/security/ssl/DefaultOpenDistroSecurityKeyStore.java

debjanibnrj · 2021-04-09T17:13:44Z

Hey @jksmth, Friendly ping to see if you need any help with this. Would like to work with you to get this in.

jksmth · 2021-04-09T18:37:58Z

Hello, yes happy to get this finished off. There are quite a few test certs that need generating for the unit tests. I'll try and make a start on it in the next few days. Any helper scripts around to help generate them or is it a manual task?

debjanibnrj · 2021-04-09T18:52:09Z

The cert generation tool mentioned here - https://docs.search-guard.com/latest/offline-tls-tool#tls-tool should help.

debjanibnrj · 2021-04-27T23:51:46Z

Hey @jksmth, Looks like this branch needs to be rebased with main

Signed-off-by: Jake Smith <jakemgsmith@gmail.com>

jksmth · 2021-05-02T11:25:35Z

Hello @debjanibnrj I have rebased and had to force-push and close/reopen this PR to get it to pickup the changes following the source repo rename to OpenSearch. Please approve the workflow if you are happy and let me know any comments, functionality and tests are now complete.

The certs were generated with the offline-tls-tool as you suggested - FYI I also have a PR into that project to add otherName support: floragunncom/search-guard-tlstool#18

codecov-commenter · 2021-05-03T17:39:37Z

Codecov Report

Merging #943 (0840977) into main (80ebcf4) will increase coverage by 0.01%.
The diff coverage is 66.66%.

@@             Coverage Diff              @@
##               main     #943      +/-   ##
============================================
+ Coverage     64.55%   64.57%   +0.01%     
- Complexity     3165     3173       +8     
============================================
  Files           244      244              
  Lines         17124    17153      +29     
  Branches       3036     3042       +6     
============================================
+ Hits          11054    11076      +22     
- Misses         4523     4528       +5     
- Partials       1547     1549       +2

Impacted Files	Coverage Δ	Complexity Δ
...curity/ssl/ExternalOpenDistroSecurityKeyStore.java	`48.00% <0.00%> (-0.98%)`	`7.00 <0.00> (ø)`
...ecurity/ssl/DefaultOpenDistroSecurityKeyStore.java	`68.93% <67.56%> (+0.21%)`	`74.00 <4.00> (+4.00)`
...ssl/rest/OpenDistroSecuritySSLCertsInfoAction.java	`61.11% <100.00%> (+2.49%)`	`5.00 <0.00> (ø)`
...ecurity/configuration/ConfigurationRepository.java	`68.85% <0.00%> (-3.83%)`	`21.00% <0.00%> (ø%)`
...ty/configuration/ConfigurationLoaderSecurity7.java	`68.85% <0.00%> (+0.81%)`	`10.00% <0.00%> (ø%)`
...earch/security/auditlog/impl/AbstractAuditLog.java	`75.72% <0.00%> (+1.58%)`	`94.00% <0.00%> (+3.00%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80ebcf4...0840977. Read the comment docs.

debjanibnrj

Thanks for your contribution!

@hardik-k-shah @vrozov Let me know if you can have a look

jksmth · 2021-07-30T14:32:05Z

Hello

Are you still interested in this PR? I notice my branch is behaving a bit strangely in GitHub as it was forked before the project rename.

Would you like me to refork, rebase and submit a new PR?

hardik-k-shah · 2021-07-30T17:07:25Z

Hello

Are you still interested in this PR? I notice my branch is behaving a bit strangely in GitHub as it was forked before the project rename.

Would you like me to refork, rebase and submit a new PR?

Yes, please resubmit it on OpenSearch repo. Thanks.

jksmth · 2021-08-03T00:17:54Z

Closing this PR - replaced by #1369

…ject#943) Signed-off-by: cliu123 <lc12251109@gmail.com> Co-authored-by: Darshit Chanpura <35282393+DarshitChanpura@users.noreply.github.com>

…arch-project#943)" (opensearch-project#950) This reverts commit 6d6c5b7. Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

jksmth mentioned this pull request Dec 30, 2020

SSL certificate hot reload with OtherName in SAN #843

Closed

debjanibnrj reviewed Jan 15, 2021

View reviewed changes

...va/com/amazon/opendistroforelasticsearch/security/ssl/DefaultOpenDistroSecurityKeyStore.java Show resolved Hide resolved

cliu123 changed the base branch from master to main February 6, 2021 00:09

Parse byte array of otherName SANs to string of ASN1 object and OID

0840977

Signed-off-by: Jake Smith <jakemgsmith@gmail.com>

jksmth closed this May 2, 2021

jksmth reopened this May 2, 2021

jksmth force-pushed the hotreload-san-othername branch from 40da1f7 to 0840977 Compare May 2, 2021 11:14

jksmth marked this pull request as ready for review May 3, 2021 12:01

debjanibnrj approved these changes May 3, 2021

View reviewed changes

jksmth force-pushed the hotreload-san-othername branch from 03e5b9f to 0840977 Compare May 9, 2021 15:06

jksmth mentioned this pull request Aug 3, 2021

Parse byte array of otherName SANs to string of ASN1 object and OID #1369

Merged

jksmth closed this Aug 3, 2021

Conversation

jksmth commented Dec 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

opendistro-for-elasticsearch/security pull request intake form

Uh oh!

jksmth commented Dec 30, 2020

Uh oh!

codecov bot commented Dec 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

debjanibnrj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

debjanibnrj commented Apr 9, 2021

Uh oh!

jksmth commented Apr 9, 2021

Uh oh!

debjanibnrj commented Apr 9, 2021

Uh oh!

debjanibnrj commented Apr 27, 2021

Uh oh!

jksmth commented May 2, 2021

Uh oh!

codecov-commenter commented May 3, 2021

Codecov Report

Uh oh!

debjanibnrj left a comment

Choose a reason for hiding this comment

Uh oh!

jksmth commented Jul 30, 2021

Uh oh!

hardik-k-shah commented Jul 30, 2021

Uh oh!

jksmth commented Aug 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jksmth commented Dec 30, 2020 •

edited

Loading

codecov bot commented Dec 30, 2020 •

edited

Loading