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

Add support for GnuTLS for hashing #1189

Closed
wants to merge 1 commit into from
Closed

Add support for GnuTLS for hashing #1189

wants to merge 1 commit into from

Conversation

jlaako
Copy link
Contributor

@jlaako jlaako commented Sep 19, 2017

Support for GnuTLS is also partially preparatory work for supporting PKCS#7 signatures using X.509 keys instead of GnuPG through gpgme.

There is also a separate build fix patch for misplaced #ifdef in the sources that incorrectly crosses control block.

@rh-atomic-bot
Copy link

Can one of the admins verify this patch?
I understand the following commands:

  • bot, add author to whitelist
  • bot, test pull request
  • bot, test pull request once

jlebon
jlebon reviewed Sep 19, 2017
View reviewed changes
src/ostree/ot-builtin-commit.c Outdated
#else
g_set_error (error, G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED,
"This version of ostree is not compiled with libarchive support");
return FALSE;
#endif
}
Copy link
Member

Choose a reason for hiding this comment

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

Note that this was fixed in #1181, so you should be able to just rebase and drop that one out.

@jlebon
Copy link
Member

jlebon commented Sep 19, 2017

bot, test pull request

@cgwalters
Copy link
Member

What's the rationale behind gnutls and libgcrypt? Which HTTP backend are you planning to use, and if it's libcurl, is it also configured for gnutls?

Can you include the PR header text in the commit message itself too? Really as much real-world information in the commit messages the better; github PR conversations may not be around 10 years later.

We'll also want to add a CI context for this; I can take care of that though.

@jlaako
Copy link
Contributor Author

jlaako commented Sep 20, 2017

@jlebon I'll rebase the PR and drop the second patch.

@cgwalters rationale for these things is that ultimately we would like to use ostree without gpgme for both GPLv3 and performance reasons. As I wrote in a mailing list post a while ago, using X.509 certificates and PKCS#7 signatures is most straightforward and standard method. For that purpose gnutls has best support and I have code for doing signing and verification with these that I now need to adapt to ostree. I was also planning to implement the same using libgcrypt (which shares the cryptographic base with gpg), but it seems quite a bit more complex to do. I anyway implemented autotools parts and hashing for both, thinking that gnutls is good for X.509/PKCS#7 and libgcrypt is good for use when using gpgme - both support hardware accelerators like AES-NI. The X.509/PKCS#7 part could also be implemented with openssl in future, but licensing wise openssl is tricky to combine (L)GPL software due to license incompatibility.

I'm not planning to make changes regarding HTTP backend part. libcurl configuration is a separate distro specific question. I personally prefer libsoup which supports gnutls and at some point I could add support for that too (of course depending on how welcome such would be)...

cgwalters
cgwalters reviewed Sep 21, 2017
View reviewed changes
src/libotutil/ot-checksum-instream.c Outdated
@@ -1,3 +1,4 @@
/* vi: set et sw=2 ts=2 cino=t0,(0: */
Copy link
Member

Choose a reason for hiding this comment

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

Let's not do this, see instead #1206 - presumably there's a vim equivalent? There's also .editorconfig which I think I used elsewhere.

Copy link
Member

Choose a reason for hiding this comment

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

There is. I upstreamed my copy at #1208. @jlaako, feel free to suggest tweaks if there's things I'm missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I hope vim knows how to deal with it. I'll remove it.

cgwalters
cgwalters reviewed Sep 21, 2017
View reviewed changes
configure.ac Outdated
@@ -346,6 +346,45 @@ if test x$with_openssl != xno; then OSTREE_FEATURES="$OSTREE_FEATURES openssl";
AM_CONDITIONAL(USE_OPENSSL, test $with_openssl != no)
dnl end openssl

dnl begin gnutls
Copy link
Member

Choose a reason for hiding this comment

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

There's currently no reason to link to more than one of (openssl, gnutls, libgcrypt) right? So maybe we should instead have --with-crypto= ? And then the default value would be glib.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can try to look into this, but I'm not so great on autotools...

Copy link
Member

Choose a reason for hiding this comment

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

No worries, I can do it.

cgwalters
cgwalters reviewed Sep 21, 2017
View reviewed changes
src/libotutil/ot-checksum-instream.c Outdated
# if defined(HAVE_GCRYPT)
g_assert (gcry_check_version (GCRYPT_VERSION));
gcry_control (GCRYCTL_DISABLE_SECMEM, 0);
gcry_control (GCRYCTL_INITIALIZATION_FINISHED, 0);
Copy link
Member

Choose a reason for hiding this comment

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

Not your fault - this bit is recommended by the docs, but I have to say this is ugly. Do we really need to do this dance to just use the checksums API?

Also, doesn't it potentially conflict with using libgpgme in process to e.g. sign things? Hopefully calling GCRYCTL_DISABLE_SECMEM doesn't like deliberately do a open('/tmp/myprivategpgkey').write(...).chmod(0644) or whatever but still...

Copy link
Member

Choose a reason for hiding this comment

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

It sounds like you are planning to use gnutls here and not libgcrypt; if that's the case can we just omit that for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, unfortunately it is necessary, otherwise some of the hash stuff fails on "make check" with GPG_ERR_NOT_OPERATIONAL...

I can remove the libgcrypt parts if it is seen best alternative. Initially gnutls seems to be most convenient way to do the certificate and signature functionality. And it is sort of supported by the fact that libsoup uses it too (when/if used with ostree).

I call DISABLE_SECMEM to avoid issues when doing containerized builds or builds on restricted environments that don't allow mlock() and similar. My thinking was that build servers are not high risk environments for key leakage and at client side public keys are not sensitive.

@rh-atomic-bot
Copy link

☔ The latest upstream changes (presumably 6e4146a) made this pull request unmergeable. Please resolve the merge conflicts.

@jlaako @cgwalters
Add --with-crypto=gnutls
f694c38 
Introduce support for GnuTLS for computing cryptograpic
hashes, similar to the OpenSSL backend.  A reason to do
this is some distributors want to avoid GPLv3, and GPG
pulls that in.

A possible extension of using GnuTLS would be replacing the GPG signing
with `PKCS#7` signatures and `X.509` keys.

We also support `--with-crypto=openssl`, which has the same effect
as `--with-openssl`, and continues to be supported.

Changes by Colin Walters <walters@verbum.org>:

 - Drop libgcrypt option for now
 - Unify buildsystem on --with-crypto

Link: https://mail.gnome.org/archives/ostree-list/2017-June/msg00002.html

Signed-off-by: Jussi Laako <jussi.laako@linux.intel.com>
@cgwalters
Copy link
Member

OK, I updated your PR. Can you take a look to see if the changes still look good to you? Feel free to reword/rewrite/extend the commit message if you want, etc.

@cgwalters cgwalters changed the title Add support for GnuTLS and libgcrypt for hashing Add support for GnuTLS for hashing Sep 22, 2017
@jlaako
Copy link
Contributor Author

jlaako commented Sep 25, 2017

Looks good to me!

@cgwalters
Copy link
Member

Great, thanks for the patch! Can you file an issue beforehand for the bigger work in #1189 (comment) if you're planning to do it anytime soon, so we can discuss architecture there? I think I mentioned this somewhere else, but one plan might be to add hooks such that signature verification could be done by a higher level app/library too. (There are advantages/disadvantages to this of course)

@rh-atomic-bot r+ f694c38

@rh-atomic-bot
Copy link

⌛ Testing commit f694c38 with merge f91acf5...

@rh-atomic-bot
Copy link

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

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

@cgwalters cgwalters cgwalters left review comments

@jlebon jlebon jlebon left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jlaako @rh-atomic-bot @jlebon @cgwalters