Skip to content
This repository has been archived by the owner on Oct 23, 2022. It is now read-only.

RSA keygen in preparation to asking /id from ipfs #95

Merged
merged 21 commits into from
Mar 12, 2020

Conversation

koivunej
Copy link
Collaborator

Sorry for the refactoring with the http being here as well, I created a config module with configuration file handling there. The rsa keygen and following import/export thing was a lot more difficult than I thought.

I made some questionable changes to ipfs::config as well, I removed it from the ifps::IpfsOptions. I made this because looking back at the ipfs::config::KeyMaterial it seems like a bad idea to extend it with Pkcs8Buffer or something similar, I am not so sure if the mutability through Options makes a lot of sense. If this solution feels worse I can present the KeyMaterial extension as well. I also thought that the "library initialization through configuration file" might not be so important use case after all, as most likely you'd like to bring your application configuration file and turn that into the ipfs::IpfsOptions.

This could be mergeable as is. I'll continue from here into spawning the background task and exposing the ipfs::Ipfs to the filters, but hopefully on a different PR.

Joonas Koivunen added 7 commits March 12, 2020 12:12
in preparation for making the library outside configurable without a
configuration file.
this should hopefully lead to ConfigFile not being needed anymore
taking on the openssl dependency is used for:

 - rsa key generation
 - from/to `pkcs#1` and `pkcs#8` transformations

Crate `rsa` was also considered but it does not support exporting keys
in any format at the moment and it's import code looks different from
`ring`s (this could be the case of not importing the "precomputed"
values in `rsa`).

The `pkcs#1` is (hopefully) used with go-ipfs-config/go-libp2p
compatible key storage but rust-libp2p/ring need the key in `pkcs#8` der
format. PEM parsing and PEM to DER was implemented by hand as it looked
like the existingly depended on crates are not compatible as much as
possible.
@koivunej koivunej requested a review from dvc94ch March 12, 2020 10:19
@koivunej koivunej changed the title Actual RSA keygen in preparation to asking /id from ipfs Mar 12, 2020
Copy link
Contributor

@dvc94ch dvc94ch left a comment

Choose a reason for hiding this comment

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

Looks very good, thanks!

// strings from go-ipfs
eprintln!("Error: ipfs configuration file already exists!");
eprintln!("Reinitializing would override your keys.");
std::process::exit(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

are the go ipfs error messages checked in the compliance test suite?

Copy link
Collaborator Author

@koivunej koivunej Mar 12, 2020

Choose a reason for hiding this comment

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

I don't think so but I tried to include the ones I found. These might be presented in the conformance test assertion failures, at least if it happens during preparation.

@dvc94ch
Copy link
Contributor

dvc94ch commented Mar 12, 2020

Is there no other way to do it except using openssl?

@koivunej
Copy link
Collaborator Author

Sadly I cannot see a way. rsa crate exists and would be suitable for our testing purposes at least (there are warnings for not having audited) but for testing purposes I'd be happy to get any key from rsa imported as ring key used by rust-libp2p. There isn't any export PKCS#{1,8} functionality in rsa I could find (even less import/export in released version, some import features have been added later) but even the latest importing seems to differ from how ring imports RSA keys. As DER functionality is not exposed by openssl either so I had to hack it, luckily it's the easiest thing done here (see pem_to_der).

I wrote more/different notes on aae24e1.

The windows github actions openssl should be doable, but I can't see what would the solution be. Trying to ask around.

@koivunej
Copy link
Collaborator Author

@dvc94ch asking to make sure you saw the ipfs::config::ConfigFile related changes in ipfs::IpfsOptions I mentioned in PR description while I research the openssl library installation:

I made some questionable changes to ipfs::config as well, I removed it from the ifps::IpfsOptions. I made this because looking back at the ipfs::config::KeyMaterial it seems like a bad idea to extend it with Pkcs8Buffer or something similar, I am not so sure if the mutability through Options makes a lot of sense.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@dvc94ch
Copy link
Contributor

dvc94ch commented Mar 12, 2020

I agree that it makes sense to make the configuration the responsibility of the user, in this case http

@koivunej
Copy link
Collaborator Author

Is there no other way to do it except using openssl?

Sadly I cannot see a way.

To elaborate on others than the rsa:

  • rustls: couldn't find key generation or export
  • ring: couldn't find key generation (but help to use openssl), not sure on exporting

I agree that it makes sense to make the configuration the responsibility of the user, in this case http

Great! Thanks for the another look.

Still no luck with the build however, there are some open issues like sfackler/rust-openssl#1062 but strange not to find examples on this.

Joonas Koivunen added 4 commits March 12, 2020 14:54
apparently these images have the openssl installed on some hosts.
this is because I don't know how to install openssl in a cross
compilable way with brew/apt-get.
@koivunej
Copy link
Collaborator Author

Trying to disable http on cross builds as I have no idea on how to start building openssl or downloading any prebuilts for the cross compilation with either apt or brew. I'll create an issue for this, I'm sure someone knows how to fix this.

Joonas Koivunen added 3 commits March 12, 2020 16:02
In addition to typoing the http crate name again I managed to remove
cross compilation on the non-android cross compilation targets.
@koivunej koivunej merged commit e5c7c08 into rs-ipfs:master Mar 12, 2020
This was referenced Mar 12, 2020
@koivunej koivunej deleted the actual_id branch March 16, 2020 11:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants