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.

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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

gevaerts
Copy link
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.

@gevaerts
nss: allow fifos and character devices for certificates.
b5cc7c7 
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
Contributor Author

This was reported on irc by pawky

@bagder
Copy link
Member

bagder commented Apr 26, 2019

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

kdudka
kdudka approved these changes Apr 26, 2019
View reviewed changes
Copy link
Contributor

@kdudka kdudka left a comment

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
Copy link

dec0de commented Apr 26, 2019 •
edited by kdudka

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
Copy link
Contributor

kdudka commented Apr 26, 2019

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
Copy link

dec0de commented Apr 29, 2019

I use bash.

@bagder
Copy link
Member

bagder commented May 1, 2019

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
Copy link
Contributor Author

gevaerts commented May 2, 2019

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
Copy link
Contributor

kdudka commented May 2, 2019

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
Copy link
Contributor

kdudka commented May 6, 2019

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

Is anybody against merging this pull request?

@bagder
Copy link
Member

bagder commented May 6, 2019

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.
Reviewers

@kdudka kdudka kdudka approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@gevaerts @bagder @dec0de @kdudka