Raise minimum bits required for RSA key to 2048

[bigs]

@raulk @Stebalien this is consistent with the go-libp2p and go-ipfs defaults, so I think this should be okay to merge. Would like a second opinion.

[Stebalien]

Unfortunately, we have a bunch of tests that rely on this working (for speed). Honestly, we should just switch those tests over to ed25519 where possible.

The second concern are the interop tests that hard-code 512 bit keys: https://github.com/ipfs/interop/blob/de333b816e29ea2c7ff8149de2069a5a28eb4307/test/repo.js

So yeah.... I think we need to a top-to-bottom code-base audit first.

[raulk]

Closes #35.

[bigs]

@Stebalien could we also pre-generate keys for those tests? if we were to go the environment variable route, i'd want it to be a flag that emphasized it's relative unsafety i.e. LIBP2P_ALLOW_UNSAFE_RSA

[Stebalien]

We could, I believe JS does that. It may slow down handshakes a bit but it should work.

Let's try the environment variable for now. We can just set that in all test environments.

[bigs]

sounds good to me. i'll check for it in init()

[raulk]

1. The env variable approach will require changes to all .travis.yml across repos.
2. I suggest replacing the term "unsafe" (which has specific connotations in programming, and also denotes binary judgement from our end safe/not-safe) by "weak".

[raulk]

Note to team: this is a BREAKING change for downstreams using 512-bit RSA keys. We want to force them away from weak cryptography, but we should:

• announce this in discuss.libp2p.io.
• add a big NOTICE in release notes.
• bump up the minor version of go-libp2p-core.

[bigs]

@raulk i'm going to move forward with this today. is it acceptable to do this in two stages, committing this minor release and announcing it on discuss.libp2p.io and then going through repos in the org and fixing broken travis files?

[raulk]

Requiring an env variable to run tests is suboptimal. I don’t want to be thinking about that when running go test on any repo.

Instead, let’s add an init function on the relevant packages that generate test identities in go-libp2p-testing. We override the global var there.

Other than that, LGTM.

Spoke to @bigs and we’re running ahead with this unless somebody objects promptly.

[Stebalien]

Instead, let’s add an init function on the relevant packages that generate test identities in go-libp2p-testing. We override the global var there.

Let's be very careful about that. It's easy to accidentally import such a package.

At the very least, we need to log LOUDLY if that happens.

[raulk]

Let’s have the init function check if it’s running in a test (e.g. https://stackoverflow.com/questions/14249217/how-do-i-know-im-running-within-go-test), and only modify the env var if it is.

[Stebalien]

FYI, that won't fix many of our integration tests. We'll still need an env variable (or we'll need to just use ed25519).

[bigs]

IMO it's safer to leave the testing infra as is and add the environment variables. I'd hate for an effects-only import to weaken our crypto constraints.

[Stebalien]

This isn't about testing infra, it's about users running go test ./... and it not working. Let's just switch to ed25519. That solves all our issues.

[raulk]

@bigs It won’t. People are not supposed to import from go-libp2p-testing in their production code. If they do, I’m ok for them to lose guarantees. We can’t solve for deliberate bad usage of our APIs, and penalise legit uses along the way.

The environment variable solution implies remembering to run go test locally as well with an env variable, which is plain horrendous.

[raulk]

@Stebalien Yeah, sounds like ed25519 is the way to go. Modifying the test identity factory functions in go-libp2p-testing should be enough. Unless there are test assertions that rely on the key being RSA, which shouldn’t be the case. @bigs

[bigs]

Okay, I'll update the testing utility and make sure there aren't any libraries bypassing its use. Starting with this release, then testing, then the rest.

[Stebalien]

I thought we agreed to not require an environment variable? I just tried running the tests locally and was very surprised when they failed.

[raulk]

@bigs could you focus on this today? What was the plan here?

[bigs]

hmm @Stebalien sorry about this. the aim was to keep the flag but not have it required for our tests to pass. i'll fix this.

[hsanjuan]

Heads up that go-ipfs won't be able to bootstrap anymore to public bootstrappers without re-allowing weak keys because bootstrappers use 1024-bit rsa keys.

[Stebalien]

That's annoying. Thanks for catching it!

(it also made me realize that we're not enforcing this with OpenSSL).