8:22 a.m.
See 2/2 for explanation.
Ideally, we wouldn't use gnutls_dh_params_generate2() at all, per [1].
But that would require bumping minimal required version to gnutls-3.6.0
and I'm not sure how available it is in OSes we support. Therefore, for
now let's stick with patch 2/2.
1: https://www.gnutls.org/manual/html_node/Parameter-generation.html
Michal Prívozník (2):
virnettlscontext: Drop gnutls_dh_set_prime_bits()
virnettlscontext: Don't pass static key length to
gnutls_dh_params_generate2()
src/rpc/virnettlscontext.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
--
2.32.0
8:22 a.m.
New subject: [PATCH 1/2] virnettlscontext: Drop gnutls_dh_set_prime_bits()
According to the gnutls_dh_set_prime_bits() manpage:
The function has no effect in server side.
Therefore, don't call it when creating server side context.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/rpc/virnettlscontext.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
index 1a3dd92676..3b6687e7f4 100644
--- a/src/rpc/virnettlscontext.c
+++ b/src/rpc/virnettlscontext.c
@@ -1233,8 +1233,6 @@ virNetTLSSession *virNetTLSSessionNew(virNetTLSContext *ctxt,
*/
if (ctxt->isServer) {
gnutls_certificate_server_set_request(sess->session, GNUTLS_CERT_REQUEST);
-
- gnutls_dh_set_prime_bits(sess->session, DH_BITS);
}
gnutls_transport_set_ptr(sess->session, sess);
--
2.32.0
10:22 a.m.
New subject: [PATCH 1/2] virnettlscontext: Drop gnutls_dh_set_prime_bits()
On Tue, 21 Dec 2021, Michal Privoznik wrote:
According to the gnutls_dh_set_prime_bits() manpage:
The function has no effect in server side.
Therefore, don't call it when creating server side context.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
Reviewed-by: Ani Sinha <ani(a)anisinha.ca>
---
src/rpc/virnettlscontext.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
index 1a3dd92676..3b6687e7f4 100644
--- a/src/rpc/virnettlscontext.c
+++ b/src/rpc/virnettlscontext.c
@@ -1233,8 +1233,6 @@ virNetTLSSession *virNetTLSSessionNew(virNetTLSContext *ctxt,
*/
if (ctxt->isServer) {
gnutls_certificate_server_set_request(sess->session, GNUTLS_CERT_REQUEST);
-
- gnutls_dh_set_prime_bits(sess->session, DH_BITS);
}
gnutls_transport_set_ptr(sess->session, sess);
--
2.32.0
6:49 a.m.
New subject: [PATCH 1/2] virnettlscontext: Drop gnutls_dh_set_prime_bits()
On a Tuesday in 2021, Michal Privoznik wrote:
According to the gnutls_dh_set_prime_bits() manpage:
The function has no effect in server side.
On top of that, it is deprecated as of 3.1.7 while our minimum version
is 3.2.
Therefore, don't call it when creating server side context.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/rpc/virnettlscontext.c | 2 --
1 file changed, 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
8:22 a.m.
New subject: [PATCH 2/2] virnettlscontext: Don't pass static key length to gnutls_dh_params_generate2()
As encryption norms get more strict it's easy to fall on the
insecure side. For instance, so far we are generating 2048 bits
long prime for Diffie-Hellman keys. Some systems consider this
not long enough. While we may just keep increasing the value
passed to the corresponding gnutls_* function, that is not well
maintainable. Instead, we may do what's recommended in the
gnutls_* manpage. From gnutls_dh_params_generate2(3):
It is recommended not to set the number of bits directly, but
use gnutls_sec_param_to_pk_bits() instead.
Looking into the gnutls_sec_param_to_pk_bits() then [1], 2048
bits corresponds to parameter MEDIUM. Therefore, we want to chose
the next size (HIGH) to be future proof.
1: https://www.gnutls.org/manual/gnutls.html#tab_003akey_002dsizes
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/rpc/virnettlscontext.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
index 3b6687e7f4..f0b1e8f9c1 100644
--- a/src/rpc/virnettlscontext.c
+++ b/src/rpc/virnettlscontext.c
@@ -38,8 +38,6 @@
#include "virthread.h"
#include "configmake.h"
-#define DH_BITS 2048
-
#define LIBVIRT_PKI_DIR SYSCONFDIR "/pki"
#define LIBVIRT_CACERT LIBVIRT_PKI_DIR "/CA/cacert.pem"
#define LIBVIRT_CACRL LIBVIRT_PKI_DIR "/CA/cacrl.pem"
@@ -718,6 +716,15 @@ static virNetTLSContext *virNetTLSContextNew(const char *cacert,
* security requirements.
*/
if (isServer) {
+ unsigned int bits = 0;
+
+ bits = gnutls_sec_param_to_pk_bits(GNUTLS_PK_DH, GNUTLS_SEC_PARAM_HIGH);
+ if (bits == 0) {
+ virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
+ _("Unable to get key length for diffie-hellman
parameters"));
+ goto error;
+ }
+
err = gnutls_dh_params_init(&ctxt->dhParams);
if (err < 0) {
virReportError(VIR_ERR_SYSTEM_ERROR,
@@ -725,7 +732,7 @@ static virNetTLSContext *virNetTLSContextNew(const char *cacert,
gnutls_strerror(err));
goto error;
}
- err = gnutls_dh_params_generate2(ctxt->dhParams, DH_BITS);
+ err = gnutls_dh_params_generate2(ctxt->dhParams, bits);
if (err < 0) {
virReportError(VIR_ERR_SYSTEM_ERROR,
_("Unable to generate diffie-hellman parameters:
%s"),
--
2.32.0
10:33 a.m.
New subject: [PATCH 2/2] virnettlscontext: Don't pass static key length to gnutls_dh_params_generate2()
On Tue, 21 Dec 2021, Michal Privoznik wrote:
As encryption norms get more strict it's easy to fall on the
insecure side. For instance, so far we are generating 2048 bits
long prime for Diffie-Hellman keys. Some systems consider this
not long enough. While we may just keep increasing the value
passed to the corresponding gnutls_* function, that is not well
maintainable. Instead, we may do what's recommended in the
gnutls_* manpage. From gnutls_dh_params_generate2(3):
It is recommended not to set the number of bits directly, but
use gnutls_sec_param_to_pk_bits() instead.
Looking into the gnutls_sec_param_to_pk_bits() then [1], 2048
bits corresponds to parameter MEDIUM. Therefore, we want to chose
the next size (HIGH) to be future proof.
1: https://www.gnutls.org/manual/gnutls.html#tab_003akey_002dsizes
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
Reviewed-by: Ani Sinha <ani(a)anisinha.ca>
---
src/rpc/virnettlscontext.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
index 3b6687e7f4..f0b1e8f9c1 100644
--- a/src/rpc/virnettlscontext.c
+++ b/src/rpc/virnettlscontext.c
@@ -38,8 +38,6 @@
#include "virthread.h"
#include "configmake.h"
-#define DH_BITS 2048
-
#define LIBVIRT_PKI_DIR SYSCONFDIR "/pki"
#define LIBVIRT_CACERT LIBVIRT_PKI_DIR "/CA/cacert.pem"
#define LIBVIRT_CACRL LIBVIRT_PKI_DIR "/CA/cacrl.pem"
@@ -718,6 +716,15 @@ static virNetTLSContext *virNetTLSContextNew(const char *cacert,
* security requirements.
*/
if (isServer) {
+ unsigned int bits = 0;
+
+ bits = gnutls_sec_param_to_pk_bits(GNUTLS_PK_DH, GNUTLS_SEC_PARAM_HIGH);
+ if (bits == 0) {
+ virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
+ _("Unable to get key length for diffie-hellman
parameters"));
+ goto error;
+ }
+
err = gnutls_dh_params_init(&ctxt->dhParams);
if (err < 0) {
virReportError(VIR_ERR_SYSTEM_ERROR,
@@ -725,7 +732,7 @@ static virNetTLSContext *virNetTLSContextNew(const char *cacert,
gnutls_strerror(err));
goto error;
}
- err = gnutls_dh_params_generate2(ctxt->dhParams, DH_BITS);
+ err = gnutls_dh_params_generate2(ctxt->dhParams, bits);
if (err < 0) {
virReportError(VIR_ERR_SYSTEM_ERROR,
_("Unable to generate diffie-hellman parameters:
%s"),
--
2.32.0
7:12 a.m.
New subject: [PATCH 2/2] virnettlscontext: Don't pass static key length to gnutls_dh_params_generate2()
On a Tuesday in 2021, Michal Privoznik wrote:
As encryption norms get more strict it's easy to fall on the
insecure side. For instance, so far we are generating 2048 bits
long prime for Diffie-Hellman keys. Some systems consider this
not long enough. While we may just keep increasing the value
passed to the corresponding gnutls_* function, that is not well
maintainable. Instead, we may do what's recommended in the
Is there a promise gnutls will increase those in the future?
gnutls_* manpage. From gnutls_dh_params_generate2(3):
It is recommended not to set the number of bits directly, but
use gnutls_sec_param_to_pk_bits() instead.
Looking into the gnutls_sec_param_to_pk_bits() then [1], 2048
bits corresponds to parameter MEDIUM. Therefore, we want to chose
the next size (HIGH) to be future proof.
IMO this patch should use MEDIUM and the bump should be separate.
1: https://www.gnutls.org/manual/gnutls.html#tab_003akey_002dsizes
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/rpc/virnettlscontext.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
8:38 a.m.
New subject: [PATCH 2/2] virnettlscontext: Don't pass static key length to gnutls_dh_params_generate2()
On Wed, Dec 22, 2021 at 02:12:37PM +0100, Ján Tomko wrote:
On a Tuesday in 2021, Michal Privoznik wrote:
>As encryption norms get more strict it's easy to fall on the
>insecure side. For instance, so far we are generating 2048 bits
>long prime for Diffie-Hellman keys. Some systems consider this
>not long enough. While we may just keep increasing the value
>passed to the corresponding gnutls_* function, that is not well
>maintainable. Instead, we may do what's recommended in the
Is there a promise gnutls will increase those in the future?
That is the whole point of the levels that they are not fixed and also
that distributions can select theirs. And the fact that you can have
one option for the whole system. Which makes me realise that we should
actually try that and not pick a level ourselves. Having said that I
should also add that I do not know how to do that. Maybe use "SYSTEM"
or "DEFAULT"?
>gnutls_* manpage. From gnutls_dh_params_generate2(3):
>
> It is recommended not to set the number of bits directly, but
> use gnutls_sec_param_to_pk_bits() instead.
>
>Looking into the gnutls_sec_param_to_pk_bits() then [1], 2048
>bits corresponds to parameter MEDIUM. Therefore, we want to chose
>the next size (HIGH) to be future proof.
IMO this patch should use MEDIUM and the bump should be separate.
>
>1: https://www.gnutls.org/manual/gnutls.html#tab_003akey_002dsizes
>
>Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>---
> src/rpc/virnettlscontext.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
2:38 a.m.
New subject: [PATCH 2/2] virnettlscontext: Don't pass static key length to gnutls_dh_params_generate2()
On 12/22/21 14:12, Ján Tomko wrote:
On a Tuesday in 2021, Michal Privoznik wrote:
> As encryption norms get more strict it's easy to fall on the
> insecure side. For instance, so far we are generating 2048 bits
> long prime for Diffie-Hellman keys. Some systems consider this
> not long enough. While we may just keep increasing the value
> passed to the corresponding gnutls_* function, that is not well
> maintainable. Instead, we may do what's recommended in the
Is there a promise gnutls will increase those in the future?
> gnutls_* manpage. From gnutls_dh_params_generate2(3):
>
> It is recommended not to set the number of bits directly, but
> use gnutls_sec_param_to_pk_bits() instead.
>
> Looking into the gnutls_sec_param_to_pk_bits() then [1], 2048
> bits corresponds to parameter MEDIUM. Therefore, we want to chose
> the next size (HIGH) to be future proof.
IMO this patch should use MEDIUM and the bump should be separate.
Good point, let me merge this with MEDIUM and post switch to HIGH in a
separate patch.
Michal
5:59 a.m.
New subject: [PATCH 2/2] virnettlscontext: Don't pass static key length to gnutls_dh_params_generate2()
On Tue, Dec 21, 2021 at 03:22:59PM +0100, Michal Privoznik wrote:
As encryption norms get more strict it's easy to fall on the
insecure side. For instance, so far we are generating 2048 bits
long prime for Diffie-Hellman keys. Some systems consider this
not long enough. While we may just keep increasing the value
passed to the corresponding gnutls_* function, that is not well
maintainable. Instead, we may do what's recommended in the
gnutls_* manpage. From gnutls_dh_params_generate2(3):
It is recommended not to set the number of bits directly, but
use gnutls_sec_param_to_pk_bits() instead.
Looking into the gnutls_sec_param_to_pk_bits() then [1], 2048
bits corresponds to parameter MEDIUM. Therefore, we want to chose
the next size (HIGH) to be future proof.
1: https://www.gnutls.org/manual/gnutls.html#tab_003akey_002dsizes
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/rpc/virnettlscontext.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
index 3b6687e7f4..f0b1e8f9c1 100644
--- a/src/rpc/virnettlscontext.c
+++ b/src/rpc/virnettlscontext.c
@@ -38,8 +38,6 @@
#include "virthread.h"
#include "configmake.h"
-#define DH_BITS 2048
-
#define LIBVIRT_PKI_DIR SYSCONFDIR "/pki"
#define LIBVIRT_CACERT LIBVIRT_PKI_DIR "/CA/cacert.pem"
#define LIBVIRT_CACRL LIBVIRT_PKI_DIR "/CA/cacrl.pem"
@@ -718,6 +716,15 @@ static virNetTLSContext *virNetTLSContextNew(const char *cacert,
* security requirements.
*/
if (isServer) {
+ unsigned int bits = 0;
+
+ bits = gnutls_sec_param_to_pk_bits(GNUTLS_PK_DH, GNUTLS_SEC_PARAM_HIGH);
+ if (bits == 0) {
+ virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
+ _("Unable to get key length for diffie-hellman
parameters"));
+ goto error;
+ }
+
err = gnutls_dh_params_init(&ctxt->dhParams);
if (err < 0) {
virReportError(VIR_ERR_SYSTEM_ERROR,
@@ -725,7 +732,7 @@ static virNetTLSContext *virNetTLSContextNew(const char *cacert,
gnutls_strerror(err));
goto error;
}
- err = gnutls_dh_params_generate2(ctxt->dhParams, DH_BITS);
+ err = gnutls_dh_params_generate2(ctxt->dhParams, bits);
if (err < 0) {
virReportError(VIR_ERR_SYSTEM_ERROR,
_("Unable to generate diffie-hellman parameters:
%s"),
We shouldn't be introducing use of gnutls_sec_param_to_pk_bits at
all IMHO, rather we should be removing use of gnutls_dh_params_generate2
instead.
The recommendation is to use pre-generated DH parameters from the
the FFDHE set of RFC7919.
In gnutls >= 3.6.0 this happens automatically.
In gnutls >= 3.5.6 && < 3.6.0 we can replace thegnutls_dh_params_generate2
+
gnutls_certificate_set_dh_params pair of calls, with just
gnutls_certificate_set_known_dh_params()
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
6:56 a.m.
New subject: [PATCH 2/2] virnettlscontext: Don't pass static key length to gnutls_dh_params_generate2()
On 1/4/22 12:59, Daniel P. Berrangé wrote:
We shouldn't be introducing use of gnutls_sec_param_to_pk_bits
at
all IMHO, rather we should be removing use of gnutls_dh_params_generate2
instead.
The recommendation is to use pre-generated DH parameters from the
the FFDHE set of RFC7919.
In gnutls >= 3.6.0 this happens automatically.
In gnutls >= 3.5.6 && < 3.6.0 we can replace thegnutls_dh_params_generate2
+
gnutls_certificate_set_dh_params pair of calls, with just
gnutls_certificate_set_known_dh_params()
Fair enough, I don't know enough about gnutls, but let me see if I can
cook a patch.
Michal
6:32 a.m.
On Tue, Dec 21, 2021 at 03:22:57PM +0100, Michal Privoznik wrote:
See 2/2 for explanation.
Ideally, we wouldn't use gnutls_dh_params_generate2() at all, per [1].
But that would require bumping minimal required version to gnutls-3.6.0
and I'm not sure how available it is in OSes we support. Therefore, for
As far as I can tell from repology.org all the major distros have 3.6.x
in more than one version and definitely all those that we have in the
CI, so I'd say bump that.
now let's stick with patch 2/2.
1: https://www.gnutls.org/manual/html_node/Parameter-generation.html
Michal Prívozník (2):
virnettlscontext: Drop gnutls_dh_set_prime_bits()
virnettlscontext: Don't pass static key length to
gnutls_dh_params_generate2()
src/rpc/virnettlscontext.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
--
2.32.0
7:14 a.m.
On a Wednesday in 2021, Martin Kletzander wrote:
On Tue, Dec 21, 2021 at 03:22:57PM +0100, Michal Privoznik wrote:
>See 2/2 for explanation.
>
>Ideally, we wouldn't use gnutls_dh_params_generate2() at all, per [1].
>But that would require bumping minimal required version to gnutls-3.6.0
>and I'm not sure how available it is in OSes we support. Therefore, for
As far as I can tell from repology.org all the major distros have 3.6.x
in more than one version and definitely all those that we have in the
CI, so I'd say bump that.
There's Ubuntu 18.04 with 3.5.18.
But we could #ifndef the old code out and use the pre-generated
parameters on every other distro, as recommended.
Jano
8:32 a.m.
On Wed, Dec 22, 2021 at 02:14:59PM +0100, Ján Tomko wrote:
On a Wednesday in 2021, Martin Kletzander wrote:
>On Tue, Dec 21, 2021 at 03:22:57PM +0100, Michal Privoznik wrote:
>>See 2/2 for explanation.
>>
>>Ideally, we wouldn't use gnutls_dh_params_generate2() at all, per [1].
>>But that would require bumping minimal required version to gnutls-3.6.0
>>and I'm not sure how available it is in OSes we support. Therefore, for
>
>As far as I can tell from repology.org all the major distros have 3.6.x
>in more than one version and definitely all those that we have in the
>CI, so I'd say bump that.
>
There's Ubuntu 18.04 with 3.5.18.
And we consider only LTS, so we can drop that in April when 20.04 is out
for 2 years. I finally found the exact spelling in docs/platform.rst
(available online at https://libvirt.org/platforms.html as well) which I
always struggle to find.
But we could #ifndef the old code out and use the pre-generated
parameters on every other distro, as recommended.
Since counting the bits is so discouraged I would also prefer this
option with the hopes for us remembering to remove that.
Actually, can we have like a commit hook that would check current date
against some file in the repository and just let us know that there
might be something to remove? O:-)
Jano
1053
days inactive
1067
days old
13 comments
6 participants
participants (6)
-
Ani Sinha -
Daniel P. Berrangé -
Ján Tomko -
Martin Kletzander -
Michal Privoznik -
Michal Prívozník