[jitsi-dev] [jitsi/libjitsi] Configurable DTLS certificate generation parameters. (#134)


#1

This pull request is a replacement of https://github.com/jitsi/libjitsi/pull/67 which is now obsolete.

Here is a sample of property settings made available and being set:
org.jitsi.impl.neomedia.transform.dtls.RSA_KEY_SIZE_CERTAINTY=80
org.jitsi.impl.neomedia.transform.dtls.RSA_KEY_SIZE=2048
org.jitsi.impl.neomedia.transform.dtls.CERT_CACHE_EXPIRE_TIME=60000
org.jitsi.impl.neomedia.transform.dtls.CERT_VALIDITY_TIME=43200000

The jitsi-meet server administrator can set the number of key bits (...RSA_KEY_SIZE), set the certificate cache expire time (...CERT_CACHE_EXPIRE_TIME=0 for the "old" way should mean new certificate on every connection), and how long the certificate is valid for after the time of generation (...CERT_VALIDITY_TIME is above set to 12 hours). Times are in milliseconds.

Defaults remain the same as in master branch.

@ibauersachs and @champtar and @lyubomir please give your feedback on what can be improved in this pull request.

I have tested the above values using a properties file only and verified via wireshark to my server. I have not yet tried cache expiry of 0 yet, though I am pretty confident that will work too.

Thank you for considering this one, and you should already have a CLA on file.

One more note: had to add getLong to ConfigUtils in order to call it. That's why it's in here.
You can view, comment on, or merge this pull request online at:

  https://github.com/jitsi/libjitsi/pull/134

-- Commit Summary --

  * Configurable DTLS certificate generation parameters.

-- File Changes --

    M src/org/jitsi/impl/neomedia/transform/dtls/DtlsControlImpl.java (165)
    M src/org/jitsi/util/ConfigUtils.java (46)

-- Patch Links --

https://github.com/jitsi/libjitsi/pull/134.patch
https://github.com/jitsi/libjitsi/pull/134.diff

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/134


#2

Hi, thanks for your contribution!
If you haven't already done so, could you please make sure you sign our CLA (https://jitsi.org/icla for individuals and https://jitsi.org/ccla for corporations)? We would unfortunately be unable to merge your patch unless we have that piece :(.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/134#issuecomment-208068603


#3

Hi @jitsi-jenkins I have already signed the CLA, thanks.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/134#issuecomment-208068892


#4

@@ -397,7 +522,7 @@ private static AsymmetricCipherKeyPair generateKeyPair()
         {
             long now = System.currentTimeMillis();
             Date notBefore = new Date(now - ONE_DAY);
- Date notAfter = new Date(now + 6 * ONE_DAY);
+ Date notAfter = new Date(now + CERT_VALIDITY_TIME);

maybe we can use ```CERT_VALIDITY_TIME + ONE_DAY``` here

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/134/files/3d1c7cb7cb91c38bf79fb48ca30c23bd3fe7f07b#r59141784


#5

         // frame. Attempt to retrieve from the cache.
         synchronized (DtlsControlImpl.class)
         {
             certificateInfo = certificateInfoCache;
             if (certificateInfo == null
- || certificateInfo.timestamp + ONE_DAY
+ || certificateInfo.timestamp + CERT_CACHE_EXPIRE_TIME

and ```CERT_VALIDITY_TIME``` here, dropping ```CERT_CACHE_EXPIRE_TIME```, and changing comments ...

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/134/files/3d1c7cb7cb91c38bf79fb48ca30c23bd3fe7f07b#r59141808


#6

         // frame. Attempt to retrieve from the cache.
         synchronized (DtlsControlImpl.class)
         {
             certificateInfo = certificateInfoCache;
             if (certificateInfo == null
- || certificateInfo.timestamp + ONE_DAY
+ || certificateInfo.timestamp + CERT_CACHE_EXPIRE_TIME

In my view, the certificate cache expiration and the certificate expiration are loosely related but really are different concepts.
What if I want the certificate to expire after one day, but I want the cache to expire after 1 minute?

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/134/files/3d1c7cb7cb91c38bf79fb48ca30c23bd3fe7f07b#r59141860


#7

         // frame. Attempt to retrieve from the cache.
         synchronized (DtlsControlImpl.class)
         {
             certificateInfo = certificateInfoCache;
             if (certificateInfo == null
- || certificateInfo.timestamp + ONE_DAY
+ || certificateInfo.timestamp + CERT_CACHE_EXPIRE_TIME

The comment change was an attempt to conform to the style standards of no more than 80 characters per line.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/134/files/3d1c7cb7cb91c38bf79fb48ca30c23bd3fe7f07b#r59141893


#8

@@ -397,7 +522,7 @@ private static AsymmetricCipherKeyPair generateKeyPair()
         {
             long now = System.currentTimeMillis();
             Date notBefore = new Date(now - ONE_DAY);
- Date notAfter = new Date(now + 6 * ONE_DAY);
+ Date notAfter = new Date(now + CERT_VALIDITY_TIME);

I suppose for the same reason as (now - ONE_DAY)?
Which I am guessing is to avoid timezone or clock setting issues, so I can see this one.

But if someone bothers to change it, why not give them the ability to choose?

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/134/files/3d1c7cb7cb91c38bf79fb48ca30c23bd3fe7f07b#r59141934


#9

         // frame. Attempt to retrieve from the cache.
         synchronized (DtlsControlImpl.class)
         {
             certificateInfo = certificateInfoCache;
             if (certificateInfo == null
- || certificateInfo.timestamp + ONE_DAY
+ || certificateInfo.timestamp + CERT_CACHE_EXPIRE_TIME

in that case why aren't you adding yet another option for ```notBefore```?

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/134/files/3d1c7cb7cb91c38bf79fb48ca30c23bd3fe7f07b#r59141961


#10

         // frame. Attempt to retrieve from the cache.
         synchronized (DtlsControlImpl.class)
         {
             certificateInfo = certificateInfoCache;
             if (certificateInfo == null
- || certificateInfo.timestamp + ONE_DAY
+ || certificateInfo.timestamp + CERT_CACHE_EXPIRE_TIME

I'm open to that idea, it just wouldn't occur to me as something to change.

I see your point though, if using + ONE_DAY, then we can re-use the cache expiration for certificate expiration as before.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/134/files/3d1c7cb7cb91c38bf79fb48ca30c23bd3fe7f07b#r59142028


#11

Is the purpose of re-using a single configuration setting to avoid allowing a conflicting pair of values that could cause trouble on the server? If so, could an assert statement with a friendly explanation of the difference solve the issue?

The purpose of disentangling the two settings would be to allow the server administrator to set the cache expiration without affecting the (already decent) default of certificate expiration.

My preference would be to be able to set the cache expiration to 0 if I want, or 2 days if I want. I see the point that if I set the cache to 100 days, then unless I also change the certificate expiration I will run into an issue.

Is that the idea?

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/134#issuecomment-208080946


#12

         // frame. Attempt to retrieve from the cache.
         synchronized (DtlsControlImpl.class)
         {
             certificateInfo = certificateInfoCache;
             if (certificateInfo == null
- || certificateInfo.timestamp + ONE_DAY
+ || certificateInfo.timestamp + CERT_CACHE_EXPIRE_TIME

when i said 'changing comments' i meant if we use only one variable we need to adjust variable comments

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/134/files/3d1c7cb7cb91c38bf79fb48ca30c23bd3fe7f07b#r59142346


#13

         // frame. Attempt to retrieve from the cache.
         synchronized (DtlsControlImpl.class)
         {
             certificateInfo = certificateInfoCache;
             if (certificateInfo == null
- || certificateInfo.timestamp + ONE_DAY
+ || certificateInfo.timestamp + CERT_CACHE_EXPIRE_TIME

Oh, understood.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/134/files/3d1c7cb7cb91c38bf79fb48ca30c23bd3fe7f07b#r59142359


#14

         // frame. Attempt to retrieve from the cache.
         synchronized (DtlsControlImpl.class)
         {
             certificateInfo = certificateInfoCache;
             if (certificateInfo == null
- || certificateInfo.timestamp + ONE_DAY
+ || certificateInfo.timestamp + CERT_CACHE_EXPIRE_TIME

What do you think of this: dropping CERT_VALIDITY_TIME and keeping CERT_CACHE_EXPIRE_TIME, and padding the validity by ONE_DAY to either side of the cache expiration?
Essentially the same idea, right? Except making clear to the admin that it is a cache that he/she is tuning. And then the software adjusts the certificate expirations to keep the admin out of trouble.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/134/files/3d1c7cb7cb91c38bf79fb48ca30c23bd3fe7f07b#r59142436


#15

I prefer simple config that always work, so having only one option help here.

Generating RSA key too often is useless as we now enforce PFS since #110.
On a webserver you regenerate your cert (including key) every month or less and nobody think it's insecure (for letsencrypt it's 90 days).
Chrome cache his cert for 30 days.

Maybe cert/cache validity should be in days and not in seconds, with a minimum of 1.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/134#issuecomment-208082678


#16

         // frame. Attempt to retrieve from the cache.
         synchronized (DtlsControlImpl.class)
         {
             certificateInfo = certificateInfoCache;
             if (certificateInfo == null
- || certificateInfo.timestamp + ONE_DAY
+ || certificateInfo.timestamp + CERT_CACHE_EXPIRE_TIME

yep, or rename it CERT_CACHE_VALIDITY_TIME?

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/134/files/3d1c7cb7cb91c38bf79fb48ca30c23bd3fe7f07b#r59142641


#17

I hear what you're saying, and I cannot come up with any cybercryptosecuritygeek reason to disagree.

My priority is to chat with friends and family, so I don't really need to worry about my precious server processor being taxed, and I don't mind it being used for extraneous RSA key generation. I have no evidence, but I just have a personal emotional feeling that since the cost of generating new RSA keys in this instance is very low to me, I feel like generating them frequently.

If I were running a server with a large user-base I would be thinking more along the terms you describe. Or if I were a proprietary software vendor I might be worried if the people I was trying to enchant had to wait 6 seconds instead of 4 seconds.

But since I am just a friends-and-family user, and since I may be wanting to spend my own server power (that I pay for) to do RSA key generation, I think it's OK to offer me and other users that option, especially since it was the default for a long time and it worked fine.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/134#issuecomment-208085425


#18

Added https://github.com/jitsi/libjitsi/pull/134/commits/de02d9ad8e2aa430c91da87c5a12dd8c407bbed2 to the PR to simplify the configuration options.

I will say that it's not perfect because the user could put in a negative value, and perhaps the units are slightly cumbersome, but the point of this pull request is to allow the user to make inefficient or seemingly stupid choices if the user so desires. The defaults are left alone, those are a different discussion.

Choices now include three configuration options: specifying bit length of RSA keys, specifying the second argument to [BigInteger](https://docs.oracle.com/javase/7/docs/api/java/math/BigInteger.html#BigInteger(int,%20int,%20java.util.Random)) during key generation, and specifying how long the cache lives.

@ibauersachs thanks for your patience on this one, I hope you will consider it.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/134#issuecomment-208092690


#19

Wait to hear from other, but i would squash the 2 commits into 1

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/134#issuecomment-208171854


#20

Last time I attempted to squash I had a bad experience. But I'm probably just doing it wrong.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/134#issuecomment-208310832