Fix P12 parsing for certificates with "Basic constraints" #122056
Conversation
jportner
added
v8.0.0
auto-backport
This fixes the root cause of the bug. I had to use a yarn dependency resolution to force-upgrade this, as it's a transitive dependency of the @elastic/request-crypto package. I'll open a separate issue in that repo to get it fixed there, too; when that's fixed and upgraded in the Kibana repo we can remove this resolution.
jportner
force-pushed
the
issue-122054-fix-p12-parsing
branch
from
b177a81
to
ad696ff
Compare
There was a problem hiding this comment.
I'd prefer to change the P12 parser to log a warning when a cert bag is empty. However, we don't have access to a logger when this is being used (while we are reading/validating the config).
Another idea I had is to debug log the fingerprints of each certificate that is loaded when they are consumed (for example, in the Elasticsearch client). That would at least make it easier to diagnose that a certificate is missing when a user is having TLS issues. However, that doesn't cover all of our bases (certificates are also used for the HTTP server).
So, I think this fix is sufficient for now.
package.json
Outdated
| @@ -86,6 +86,7 @@ | |||
| "**/istanbul-lib-coverage": "^3.2.0", | |||
| "**/json-schema": "^0.4.0", | |||
| "**/minimist": "^1.2.5", | |||
| "**/node-jose": "^1.1.4", | |||
There was a problem hiding this comment.
As mentioned in ad696ff, I had to use a yarn dependency resolution to upgrade this transitive dependency.
We depend on node-jose via @elastic/request-crypto, so I submitted another PR there to upgrade that dependency: elastic/request-crypto#11
Once that is merged and a new version is deployed to the npm registry, I can remove this dependency resolution. I wasn't sure how long that would take but it looks like that might be done tomorrow, so I might be able to upgrade that instead and remove this dependency resolution before merging this PR.
There was a problem hiding this comment.
Both options sounds good to me 👍
There was a problem hiding this comment.
Updated to @elastic/request-crypto 2.0.0 and node-jose 2.0.0, and removed the dependency resolutions, in fa4eecc.
|
ACK: reviewing... |
package.json
Outdated
| @@ -86,6 +86,7 @@ | |||
| "**/istanbul-lib-coverage": "^3.2.0", | |||
| "**/json-schema": "^0.4.0", | |||
| "**/minimist": "^1.2.5", | |||
| "**/node-jose": "^1.1.4", | |||
There was a problem hiding this comment.
Both options sounds good to me 👍
Also updates node-jose to 2.0.0. The reason for the major version change is that it drops support for Node 8. By doing this we were able to remove two dependency resolutions in the package.json file.
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
💔 Backport failed
To backport manually run: |
…2056) # Conflicts: # yarn.lock
…2056) # Conflicts: # yarn.lock
jportner
added
v7.16.3
auto-backport
💔 Backport failed
To backport manually run: |
…2056) # Conflicts: # yarn.lock
KOTungseth
added
the
Team:Security
|
Pinging @elastic/kibana-security (Team:Security) |
Resolves #122054.
See that issue for a detailed description.
To test, use the P12 file provided in that issue's "Steps to reproduce" section, and verify that Kibana starts without any errors.