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

crypto: expose certificate decoding function #30675

Closed
wants to merge 1 commit into from
Closed

crypto: expose certificate decoding function #30675

wants to merge 1 commit into from

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Nov 26, 2019 •
edited by jasnell

Format is the same as:

No docs or tests yet, @nodejs/crypto, I'll finish this if we want it.

Its easy, it just exposes current data format of the tls APIs, and makes testing whether certificates can be decoded quite easy, rather than having to round-trip them through TLS just to get a parsed cert :-(.

Maybe some other format would be better, and then it could be added to tls and crypto, but starting with a "better" format in crypto that is different from what tls does seems like it would increase inconsistency.

Fixes: #29181

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Nov 26, 2019
mscdex
mscdex reviewed Nov 26, 2019
View reviewed changes
src/node_crypto.cc
@@ -1153,7 +1153,6 @@ static void IsExtraRootCertsFileLoaded(
return args.GetReturnValue().Set(extra_root_certs_loaded);
}


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unintended change?

@tniessen
Copy link
Member

I am not a big fan of this format, but I am afraid we might have to stick to it.
Same goes for the function name, parseCert, but I see that we already use cert in lots of places.

@sam-github
Copy link
Contributor Author

@tniessen Can you think of a better name?

So, a couple +'s, so I should finish this off?

@sam-github
Copy link
Contributor Author

And yeah, I don't love the legacy format either, but its what we have until someone adds another.

@jasnell
Copy link
Member

jasnell commented Nov 27, 2019

+1 to getting this in. For future changes, perhaps add an options argument that accepts a format option whose only value now is legacy. This gives us some flexibility in adding an improved format as a follow on.

bnoordhuis
bnoordhuis approved these changes Nov 28, 2019
View reviewed changes
src/node_crypto.cc
CHECK(args[0]->IsArrayBufferView());
ArrayBufferViewContents<unsigned char> buf(args[0].As<ArrayBufferView>());
const unsigned char* data = buf.data();
unsigned data_len = buf.length();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of width/sign issues here: buf.length() returns a size_t, d2i_X509 takes an int. Suggestion:

size_t data_len = buf.length();
CHECK_LE(data_len, INT_MAX);

src/node_crypto.cc
X509Pointer der(d2i_X509(nullptr, &data, data_len));
if (der) {
args.GetReturnValue().Set(X509ToObject(env, der.get()));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/local consistency: no braces, ditto on line 2219.

@tniessen
Copy link
Member

Can you think of a better name?

In general, I think JavaScript and Node.js are leaning towards verbosity, so I think parseCertificate might be a better fit than parseCert. IIRC X.509 also refers to the ASN.1 structure as "Certificate", so that would align nicely. On the other hand, we already use certain naming schemes in TLS/crypto, and we do use the shorthand "cert" in other places. If that wasn't the case, I would prefer parseCertificate, but given this situation, I am +0 on the longer name.

@sam-github
Copy link
Contributor Author

Existing uses of cert are for non-X.509, so maybe using the short name will confuse people. I will make the name longer and add the docs and tests.

@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 30, 2019
@BridgeAR BridgeAR added the wip Issues and PRs that are still a work in progress. label Dec 25, 2019
@sam-github sam-github mentioned this pull request May 13, 2020
Closed
@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19
@sam-github sam-github closed this Jun 3, 2020
@sam-github sam-github deleted the parse-cert branch June 3, 2020 15:03
@baparham
Copy link

@sam-github did this get closed for any particular reason? It's not merged in yet, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mscdex mscdex mscdex left review comments

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@addaleax addaleax addaleax approved these changes

@devnexen devnexen devnexen approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. semver-minor PRs that contain new features and should be released in the next minor version. wip Issues and PRs that are still a work in progress.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

crypto: expose tls's x509 Certificate Object
10 participants
@sam-github @tniessen @jasnell @baparham @mscdex @bnoordhuis @addaleax @devnexen @BridgeAR @nodejs-github-bot