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

Add support for pkcs11 URIs in TLS client key/certificate #1183

Closed
wants to merge 2 commits into from

Conversation

OYTIS
Copy link

@OYTIS OYTIS commented Sep 18, 2017

Unfortunately I don't know how to do it for libsoup (or if it's even possible), so this part is missing.

This patch supports the respective change to Aktualizr.

@cgwalters
Copy link
Member

Please match the existing GNU code style (brace on newline, space between identifier and paren etc.)

Offhand this seems reasonable - but do you have any links for more information about this? Preferably patches to existing applications. Once we have those links let's also add them in a code comment above.

Some quick searching found https://bugzilla.redhat.com/show_bug.cgi?id=1236526 that was part of a Fedora effort to enable these types of URIs; I'm not sure how far it got.

I'm not an expert in this area but my brief understanding was that gnutls was farthest ahead here...what TlS library did you build libcurl with?

I'm guessing offhand that you want to make use of either a hardware crypto or a distinct software enclave; if you can share that information I think it'd be useful.

Unit testing this is going to be interesting...maybe https://github.com/p11-glue/p11-kit/tree/master/p11-kit has some scaffolding we can use?

@OYTIS
Copy link
Author

OYTIS commented Sep 18, 2017

@cgwalters

Please match the existing GNU code style (brace on newline, space between identifier and paren etc.)

Done (hopefully).

Offhand this seems reasonable - but do you have any links for more information about this? Preferably patches to existing applications. Once we have those links let's also add them in a code comment above.

The patch for aktualizr is work in progress, I'll make clean-ups and add the link to the PR here.

I'm not an expert in this area but my brief understanding was that gnutls was farthest ahead here...what TlS library did you build libcurl with?

I was working with OpenSSL + engine_pkcs11, just because OpenSSL has their nice tool for debugging. But it shouldn't theoretically depend on TLS backend.

I'm guessing offhand that you want to make use of either a hardware crypto or a distinct software enclave; if you can share that information I think it'd be useful.

Right now we're using SoftHSM for testing. The actual hardware module is not yet decided, we hope to support all of them as long as they have pkcs11 interface. The whole end-to-end testing harness (apart from aktualizr we need to patch our yocto/OE layer, meta-updater) is not yet finalized as well. But it involves importing key/certificate pair into SoftHSM.

Unit testing this is going to be interesting...maybe https://github.com/p11-glue/p11-kit/tree/master/p11-kit has some scaffolding we can use?

Good idea. I was using SoftHSM to test the whole stuff (aktualizr + libostree), but we can take a closer look at p11-kit

@OYTIS
Copy link
Author

OYTIS commented Sep 19, 2017

@cgwalters
I've added the link to Aktualizr's PR to the initial comment.
I'm still not sure about unit testing. SoftHSM still seems to be the most straightforward approach, but I don't know how integrate this with OSTree's test harness. I'll take a closer look at the latter.

@@ -747,6 +747,16 @@ initiate_next_curl_request (FetcherRequest *req,

if (self->tls_client_cert_path)
{
if (!strncmp (self->tls_client_key_path, "pkcs11:", 7))
Copy link
Member

Choose a reason for hiding this comment

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

Note there's a nice g_str_has_prefix function for this!

Copy link
Author

Choose a reason for hiding this comment

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

Thanks!

@cgwalters
Copy link
Member

I was looking at gnutls for a different PR and happened to see they have https://gitlab.com/gnutls/gnutls/tree/master/tests/pkcs11 - I only glanced at it though.

@cgwalters
Copy link
Member

OK, we don't need to block on figuring out unit testing for this yet. I'll assume you've tested it locally though.

I added a fixup commit that adds a comment with more information.

@rh-atomic-bot r+ d00514a

Would appreciate though of course if you get a chance to drop in any more information about the test framework you're using.

BTW, at least for flatpak we started building and running a stable flatpak snapshot as a CI context; we could consider doing the same for aktualizr. Offhand I don't know how involved that'd be, it involves some maintenance work but the obvious win is that we don't have to duplicate the tests.

@rh-atomic-bot
Copy link

⌛ Testing commit d00514a with merge db10bf4...

@rh-atomic-bot
Copy link

☀️ Test successful - status-atomicjenkins
Approved by: cgwalters
Pushing db10bf4 to master...

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.

None yet

4 participants