GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — src: eliminate ManagedEVPPkey by jasnell · Pull Request #54751 · nodejs/node

Conversation

@jasnell

@jasnell jasnell commented Sep 4, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot

nodejs-github-bot commented Sep 4, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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 involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Sep 4, 2024
@jasnell
jasnell force-pushed the eliminate-managedevppkey branch from 2ab5842 to 5e64139 Compare September 4, 2024 17:06
@jasnell
jasnell requested review from anonrig, mcollina and tniessen September 4, 2024 17:08
@jasnell
jasnell marked this pull request as ready for review September 4, 2024 17:10
anonrig
anonrig reviewed Sep 4, 2024
View reviewed changes
Comment thread src/crypto/crypto_dsa.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/crypto/crypto_ec.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/crypto/crypto_keygen.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/crypto/crypto_keygen.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/crypto/crypto_keys.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/crypto/crypto_sig.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
@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
anonrig
anonrig approved these changes Sep 4, 2024
View reviewed changes
@tniessen

tniessen commented Sep 4, 2024

Copy link
Copy Markdown
Member

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Sep 4, 2024

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tniessen

tniessen commented Sep 4, 2024

Copy link
Copy Markdown
Member

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 :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Sep 4, 2024

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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 force-pushed the eliminate-managedevppkey branch from ab5fcbf to a413907 Compare September 8, 2024 19:10
@jasnell jasnell added the author ready PRs with CI started, the required approvals, and no outstanding review comments. 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.
@jasnell
jasnell force-pushed the eliminate-managedevppkey branch from a413907 to 99dcd77 Compare September 9, 2024 17:42
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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
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
v22.9.0 proposal #54966
Merged
@targos targos added the dont-land-on-v20.x label Sep 22, 2024
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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. 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.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@jasnell @nodejs-github-bot @tniessen @anonrig @targos
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.