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

Improve URI management #230

Merged
merged 7 commits into from
Apr 19, 2023
Merged

Improve URI management #230

merged 7 commits into from
Apr 19, 2023

Conversation

simo5
Copy link
Member

@simo5 simo5 commented Apr 18, 2023

Overhaul how URI are parsed.
Add missing attributes specified in RFC 7512
Print URIs instead of ID/Label in text encoders

@simo5
Copy link
Member Author

simo5 commented Apr 18, 2023

@mouse07410 you may want to try this PR and execute:

openssl storeutl -keys -text pkcs11:

This way you will see exactly what URI the pkcs11-provider reconstruct from looking at keys.
You can use model/manufacturer/token/serial pkcs11 attributes to print only the keys of a specific token.

@simo5
Copy link
Member Author

simo5 commented Apr 18, 2023

Note that although I added support for recognizing these additional pkcs11 URI attributes:

  • library-description
  • library-manufacturer
  • library-version
  • slot-description
  • slot-id
  • slot-manufacturer

At the moment they are ignored and can't be used to filter down keys.

I am on the fence on whether I should allow all them to be used.
I see nothing wrong in allowing it at least for the slot ones (except slot id ..).
It is a bit silly to do it for the library ones, but if it turns out to be easy I'll do that as well.
Neither slot or library attributes are use to print out URIs, they are uncommon and rarely useful attributes anyway.
Only Token and Object attributes are utilized to print object URIs.

src/util.c Outdated Show resolved Hide resolved
Copy link
Contributor

@Jakuje Jakuje left a comment

Choose a reason for hiding this comment

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

lgtm.
Maybe notes regarding test coverage:

  • the uri parsing functions would be grate candidate for some extensive unit tests
  • can we have some automated tests with the openssl storeutl to check the reported uri as a whole is sensible?

@simo5
Copy link
Member Author

simo5 commented Apr 19, 2023

Yeah, I will hold on merging until I have a test specific to URIs.
Just need to decide what should be tested, if you have ideas I would like to know.

@Jakuje
Copy link
Contributor

Jakuje commented Apr 19, 2023

I would go ahead with some corner cases, percent encoding, non-printable bytes when using the storeutil and grepping for the known parts of the uris in the outputs. No more specific thoughts.

@dengert
Copy link
Contributor

dengert commented Apr 19, 2023

Just need to decide what should be tested, if you have ideas I would like to know.

@Jakuje could @simo5 use the regress/unittests/pkcs11/tests.c you wrote in 2017 for OpenSSH as discussed in OpenSC:
OpenSC/OpenSC#2741 (comment) and I was trying to port your patch to OpenSSH current It has some 56 tests.

https://github.com/dengert/openssh-portable/blob/pkcs11-URI/regress/unittests/pkcs11/tests.c

@Jakuje
Copy link
Contributor

Jakuje commented Apr 19, 2023

Sure. Thats a good idea. I would not say it is complete coverage or it is perfect, but it can be certainly used as a start or at least for ideas for some weird uris I could think of back in that time.

simo5 added 5 commits April 19, 2023 13:16
CID 452340, CID 452341, CID 452342

Signed-off-by: Simo Sorce <simo@redhat.com>
Add all RFC defined attributes

Signed-off-by: Simo Sorce <simo@redhat.com>
Any other parameter of the attribute is not appropriate to check if
there is a valid value.

Signed-off-by: Simo Sorce <simo@redhat.com>
This replaces showing just ID and Label on their own as it is a more
useful tool to exactly identify a key for future use.

Signed-off-by: Simo Sorce <simo@redhat.com>
Signed-off-by: Simo Sorce <simo@redhat.com>
@simo5
Copy link
Member Author

simo5 commented Apr 19, 2023

We do not really have unit tests at this stage yet, mostly because the vast majority of the code requires a configured token to work at this time.

So I'll add some testing just to make sure the convesion back and forth works.

@dengert
Copy link
Contributor

dengert commented Apr 19, 2023

The OpenSSH and regress/unittests/pkcs11/tests.c does not test against a token. But there are a lot of tests of making URIs.
The tests are run as part of actions/workflow https://github.com/openssh/openssh-portable/blob/master/.github/workflows/c-cpp.yml

OpenSC sets up a virtual PIV token https://github.com/OpenSC/OpenSC/blob/master/.github/test-piv.sh
and virtual OpenPGP token. https://github.com/OpenSC/OpenSC/blob/master/.github/test-openpgp.sh
Could also use SoftHSM

@Jakuje is familiar with the OpenSC actions. I only look at them if something fails.

Signed-off-by: Simo Sorce <simo@redhat.com>
@simo5
Copy link
Member Author

simo5 commented Apr 19, 2023

@Jakuje I did what I could using just openssl storeutl as a proxy to probe URIs.

Perhaps in future we'll be able to add more complete unit tests, but it looks fine for nopw, and this test will catch simple regressions.

Signed-off-by: Simo Sorce <simo@redhat.com>
@simo5 simo5 changed the title Improver URI management Improve URI management Apr 19, 2023
@simo5
Copy link
Member Author

simo5 commented Apr 19, 2023

I think I am happy with this the way it is for now

@simo5 simo5 merged commit 85b25e4 into latchset:main Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants