Skip to content
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.

Sign up for GitHub

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

Jump to bottom

src: eliminate ManagedEVPPkey #54751

Closed
wants to merge 1 commit into from
Closed

src: eliminate ManagedEVPPkey #54751

wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Sep 4, 2024 •
edited
Loading

Simplify key handling a bit by eliminating the additional indirection through ManagedEVPPKey

Prior to this change we had:

KeyObjectHandle -> std::shared_ptr<KeyObjectData> -> ManagedEVPPKey -> EVPKeyPointer

The ManagedEVPPKey really didn't add much value in the mix. After this change we have:

KeyObjectHandle -> KeyObjectData -> EVPKeyPointer

The KeyObjectData class now handles the std::shared_ptr bits internally and assumes what little responsibility the ManagedEVPPKey class had.

Overall it ends up simplifying the codebase quite a bit even if the changes are scattered across a wide area.

This is being done as part of the larger effort to move crypto logic out to the ncrypto dep.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Sep 4, 2024
@jasnell jasnell marked this pull request as ready for review September 4, 2024 17:10
anonrig
anonrig reviewed Sep 4, 2024
View reviewed changes
src/crypto/crypto_dsa.cc Show resolved Hide resolved
src/crypto/crypto_ec.cc Outdated Show resolved Hide resolved
src/crypto/crypto_keygen.h Outdated Show resolved Hide resolved
src/crypto/crypto_keygen.h Show resolved Hide resolved
src/crypto/crypto_keys.h Outdated Show resolved Hide resolved
src/crypto/crypto_sig.cc Outdated Show resolved Hide resolved
@jasnell jasnell requested a review from anonrig September 4, 2024 18:46
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@tniessen
Copy link
Member

tniessen commented Sep 4, 2024

Thanks for working on simplifying this @jasnell! I just want to make sure my understanding is correct.

IIRC, I added these types some years ago. From what I remember, ManagedEVPPKey was supposed to be a more efficient alternative to a std::shared_ptr by relying on OpenSSL's internal reference counting. Much later, I think, I added the KeyObjectData type to allow sharing KeyObject instances across threads. I suspect this pretty much eliminated the need for ManagedEVPPKey, because all instances are either uniquely owned or wrapped in a std::shared_ptr<KeyObjectData>.

Does that seem accurate to you?

@jasnell
Copy link
Member Author

jasnell commented Sep 4, 2024

Well, further simplification can be made here. The KeyObjectData holds a shared_ptr internally just for the ease of things since it also has to deal with symmetric key data. I think the slightly more inefficient shared_ptr usage is worth the e reduction in complexity but I can be swayed to make further changes here.

@tniessen
Copy link
Member

tniessen commented Sep 4, 2024

I am not asking for further simplification -- I am just trying to understand my own original motivation behind ManagedEVPPKey, which I guess predates the use of shared_ptr here. It's been too long for me to be certain :)

@jasnell
Copy link
Member Author

jasnell commented Sep 4, 2024

Yeah I've been having to refresh my memory on a lot of this also lol. It's been a good exercise. There's really tons of opportunity to simplify things in here. Moving things out to ncrypto is a great motivator to go through in detail.

@codecov Codecov

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@jasnell jasnell force-pushed the eliminate-managedevppkey branch 2 times, most recently from 3e12b70 to ab5fcbf Compare September 7, 2024 14:14
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 8, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@jasnell
src: eliminate ManagedEVPPkey
99dcd77 
Prior to this change, the ManagedEVPPkey class added an
additional layer of abstraction to the EVP_PKEY class
that wasn't strictly necessary.

Previously we had:

  KeyObjectHandle ->
      std::shared_ptr<KeyObjectData> ->
          ManagedEVPPkey ->
              EVPKeyPointer

After this change we have:

  KeyObjectHandle ->
      KeyObjectData ->
          EVPKeyPointer

The `KeyObjectData` class no longer needs to be wrapped in
std::shared_ptr but it will hold the underlying EVPKeyPointer
in a std::shared_ptr.

This greatly simplifies the abstraction and provides an overall
reduction in code and complexity, although the changeset in this
PR is fairly extensive to get there.

This refactor is being done to simplify the codebase as part
of the process of extracting crypto functionality to the
separate ncrypto dep.
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 9, 2024 •
edited by jasnell
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/62211/ 💛 FINALLY!

jasnell added a commit that referenced this pull request Sep 11, 2024
@jasnell
src: eliminate ManagedEVPPkey
285c6a0 
Prior to this change, the ManagedEVPPkey class added an
additional layer of abstraction to the EVP_PKEY class
that wasn't strictly necessary.

Previously we had:

  KeyObjectHandle ->
      std::shared_ptr<KeyObjectData> ->
          ManagedEVPPkey ->
              EVPKeyPointer

After this change we have:

  KeyObjectHandle ->
      KeyObjectData ->
          EVPKeyPointer

The `KeyObjectData` class no longer needs to be wrapped in
std::shared_ptr but it will hold the underlying EVPKeyPointer
in a std::shared_ptr.

This greatly simplifies the abstraction and provides an overall
reduction in code and complexity, although the changeset in this
PR is fairly extensive to get there.

This refactor is being done to simplify the codebase as part
of the process of extracting crypto functionality to the
separate ncrypto dep.

PR-URL: #54751
Reviewed-By: Yagiz Nizipli <[email protected]>
@jasnell jasnell closed this Sep 11, 2024
aduh95 pushed a commit that referenced this pull request Sep 12, 2024
@jasnell @aduh95
src: eliminate ManagedEVPPkey
3d689a9 
Prior to this change, the ManagedEVPPkey class added an
additional layer of abstraction to the EVP_PKEY class
that wasn't strictly necessary.

Previously we had:

  KeyObjectHandle ->
      std::shared_ptr<KeyObjectData> ->
          ManagedEVPPkey ->
              EVPKeyPointer

After this change we have:

  KeyObjectHandle ->
      KeyObjectData ->
          EVPKeyPointer

The `KeyObjectData` class no longer needs to be wrapped in
std::shared_ptr but it will hold the underlying EVPKeyPointer
in a std::shared_ptr.

This greatly simplifies the abstraction and provides an overall
reduction in code and complexity, although the changeset in this
PR is fairly extensive to get there.

This refactor is being done to simplify the codebase as part
of the process of extracting crypto functionality to the
separate ncrypto dep.

PR-URL: #54751
Reviewed-By: Yagiz Nizipli <[email protected]>
aduh95 pushed a commit that referenced this pull request Sep 13, 2024
@jasnell @aduh95
src: eliminate ManagedEVPPkey
f1eec3f 
Prior to this change, the ManagedEVPPkey class added an
additional layer of abstraction to the EVP_PKEY class
that wasn't strictly necessary.

Previously we had:

  KeyObjectHandle ->
      std::shared_ptr<KeyObjectData> ->
          ManagedEVPPkey ->
              EVPKeyPointer

After this change we have:

  KeyObjectHandle ->
      KeyObjectData ->
          EVPKeyPointer

The `KeyObjectData` class no longer needs to be wrapped in
std::shared_ptr but it will hold the underlying EVPKeyPointer
in a std::shared_ptr.

This greatly simplifies the abstraction and provides an overall
reduction in code and complexity, although the changeset in this
PR is fairly extensive to get there.

This refactor is being done to simplify the codebase as part
of the process of extracting crypto functionality to the
separate ncrypto dep.

PR-URL: #54751
Reviewed-By: Yagiz Nizipli <[email protected]>
aduh95 pushed a commit that referenced this pull request Sep 13, 2024
@jasnell @aduh95
src: eliminate ManagedEVPPkey
5e72bd3 
Prior to this change, the ManagedEVPPkey class added an
additional layer of abstraction to the EVP_PKEY class
that wasn't strictly necessary.

Previously we had:

  KeyObjectHandle ->
      std::shared_ptr<KeyObjectData> ->
          ManagedEVPPkey ->
              EVPKeyPointer

After this change we have:

  KeyObjectHandle ->
      KeyObjectData ->
          EVPKeyPointer

The `KeyObjectData` class no longer needs to be wrapped in
std::shared_ptr but it will hold the underlying EVPKeyPointer
in a std::shared_ptr.

This greatly simplifies the abstraction and provides an overall
reduction in code and complexity, although the changeset in this
PR is fairly extensive to get there.

This refactor is being done to simplify the codebase as part
of the process of extracting crypto functionality to the
separate ncrypto dep.

PR-URL: #54751
Reviewed-By: Yagiz Nizipli <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Sep 16, 2024
Merged
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 22, 2024
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
@jasnell @louwers
src: eliminate ManagedEVPPkey
b68c86a 
Prior to this change, the ManagedEVPPkey class added an
additional layer of abstraction to the EVP_PKEY class
that wasn't strictly necessary.

Previously we had:

  KeyObjectHandle ->
      std::shared_ptr<KeyObjectData> ->
          ManagedEVPPkey ->
              EVPKeyPointer

After this change we have:

  KeyObjectHandle ->
      KeyObjectData ->
          EVPKeyPointer

The `KeyObjectData` class no longer needs to be wrapped in
std::shared_ptr but it will hold the underlying EVPKeyPointer
in a std::shared_ptr.

This greatly simplifies the abstraction and provides an overall
reduction in code and complexity, although the changeset in this
PR is fairly extensive to get there.

This refactor is being done to simplify the codebase as part
of the process of extracting crypto functionality to the
separate ncrypto dep.

PR-URL: nodejs#54751
Reviewed-By: Yagiz Nizipli <[email protected]>
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
@jasnell @tpoisseau
src: eliminate ManagedEVPPkey
7e0d76c 
Prior to this change, the ManagedEVPPkey class added an
additional layer of abstraction to the EVP_PKEY class
that wasn't strictly necessary.

Previously we had:

  KeyObjectHandle ->
      std::shared_ptr<KeyObjectData> ->
          ManagedEVPPkey ->
              EVPKeyPointer

After this change we have:

  KeyObjectHandle ->
      KeyObjectData ->
          EVPKeyPointer

The `KeyObjectData` class no longer needs to be wrapped in
std::shared_ptr but it will hold the underlying EVPKeyPointer
in a std::shared_ptr.

This greatly simplifies the abstraction and provides an overall
reduction in code and complexity, although the changeset in this
PR is fairly extensive to get there.

This refactor is being done to simplify the codebase as part
of the process of extracting crypto functionality to the
separate ncrypto dep.

PR-URL: nodejs#54751
Reviewed-By: Yagiz Nizipli <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anonrig anonrig anonrig approved these changes

@mcollina mcollina Awaiting requested review from mcollina

@tniessen tniessen Awaiting requested review from tniessen

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jasnell @nodejs-github-bot @tniessen @anonrig @targos