Skip to content

nss: allow fifos and character devices for certificates. - #3807

Closed
gevaerts wants to merge 1 commit into
curl:masterfrom
gevaerts:nss-fifo-fix
Closed

nss: allow fifos and character devices for certificates.#3807
gevaerts wants to merge 1 commit into
curl:masterfrom
gevaerts:nss-fifo-fix

Conversation

@gevaerts

Copy link
Copy Markdown
Contributor

Currently you can do things like --cert <(cat ./cert.crt) with (at least) the
openssl backend, but that doesn't work for nss because is_file rejects fifos.

I don't actually know if this is sufficient, nss might do things internally
(like seeking back) that make this not work, so actual testing is needed.

Currently you can do things like --cert <(cat ./cert.crt) with (at least) the
openssl backend, but that doesn't work for nss because is_file rejects fifos.

I don't actually know if this is sufficient, nss might do things internally
(like seeking back) that make this not work, so actual testing is needed.
@gevaerts

Copy link
Copy Markdown
Contributor Author

This was reported on irc by pawky

@bagder

bagder commented Apr 26, 2019

Copy link
Copy Markdown
Member

Seems like a fine fix to me. Can you think of any reason this could be bad @kdudka ?

@kdudka kdudka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am fine with the proposed code change. Unfortunately, I do not think it will make loading of certificates from special files work as expected. The following code in nss-pem expects the file (data) size to be known before reading the data:

https://github.com/kdudka/nss-pem/blob/5c05ed26/src/util.c#L93

@dec0de

dec0de commented Apr 26, 2019

Copy link
Copy Markdown

FYI
just to clarify why one might want to use <( ):
script.sh:

read -r -d '' CERT <<CLIENT_CERT
___BEGIN CERT__
MMsfljkjvofjvoe etc...
___END CERT___
CLIENT_CERT

curl --cert <( echo -e "$CERT")   ....

This way you can have have the certs (and key) in your script not needing to keep track of several files.

@kdudka

kdudka commented Apr 26, 2019

Copy link
Copy Markdown
Contributor

Which shell interpreter are you using?

For example zsh provides the =(...) syntax exactly for this -- see the zshexpn(1) man page:

If =(...) is used instead of <(...), then the file passed as an argument will be the name of a temporary file containing the output of the list process. This may be used instead of the < form for a program that expects to lseek (see lseek(2)) on the input file.

@dec0de

dec0de commented Apr 29, 2019

Copy link
Copy Markdown

I use bash.

@bagder

bagder commented May 1, 2019

Copy link
Copy Markdown
Member

I don't mind this fix either, but as @kdudka points out I don't see how it will help as there's a file size requirement within NSS itself...

@gevaerts

gevaerts commented May 2, 2019

Copy link
Copy Markdown
Contributor Author

I agree, if it doesn't actually change anything, it doesn't make much sense to apply it.

The only thing I can think of is that maybe it changes the error message

@kdudka

kdudka commented May 2, 2019

Copy link
Copy Markdown
Contributor

I was able to remove the limitation from nss-pem: kdudka/nss-pem#4

Still it might not work as expected when the file given by --cacert is accessed repeatedly, which seems to be the case when --location takes effect.

@kdudka

kdudka commented May 6, 2019

Copy link
Copy Markdown
Contributor

I have pushed the proposed change to nss-pem: kdudka/nss-pem@651e0f03

Is anybody against merging this pull request?

@bagder

bagder commented May 6, 2019

Copy link
Copy Markdown
Member

Not me, I'm fine with merging it!

@kdudka kdudka closed this in 191ffd0 May 7, 2019
@gevaerts
gevaerts deleted the nss-fifo-fix branch June 4, 2019 12:00
@lock lock Bot locked as resolved and limited conversation to collaborators Sep 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants