Skip to content

fix load the SSL key as certificate and add documentation about generate keys #410

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
Merged
merged 1 commit into from
Mar 18, 2025
Merged

fix load the SSL key as certificate and add documentation about generate keys #410

merged 1 commit into from
Mar 18, 2025

Conversation

@qlrd
Copy link
Contributor

@qlrd qlrd commented Mar 12, 2025 •
edited
Loading

fix #409.

What is the purpose of this pull request?

  • Bug fix
  • Documentation update
  • New feature
  • Test
  • Other:

Which crates are being modified?

  • floresta-chain
  • floresta-cli
  • floresta-common
  • floresta-compact-filters
  • floresta-electrum
  • floresta-watch-only
  • floresta-wire
  • floresta
  • florestad
  • Other: .

Description

The florestad was loading the SSL key as certificate and not as a key due to the variable key_path load it as self.config.ssl_cert_path and not as self.config.ssl_key_path.

This commit fix this and also add a little docstring to indicate the user/developer how to generate them (supposed) correctly.

Notes to the reviewers

  • @lucad70 helped, personally, with the process (he found the issued function while helped him search another bug in Feat/enable backfill #215). Working together was amazing experience.

  • Also, some deepseek help that have nice comments:

There's a typo where both cert_path and key_path use ssl_cert_path. Fix the key_path to use ssl_key_path

Looking at the code, the pkcs8_private_keys function is supposed to read private keys from the key file. If that's returning an empty list, it means the key wasn't parsed correctly. The user generated the key with OpenSSL using -newkey rsa:4096, which by default creates a PEM-encoded PKCS#1 private key, not PKCS#8. The Rust code is using pkcs8_private_keys, which expects PKCS#8 format. That's probably the issue.

So the problem is the key format mismatch. The OpenSSL command generates a PKCS#1 key, but the Rust code is trying to read PKCS#8. To fix this, the user needs to either convert the key to PKCS#8 or adjust the Rust code to read PKCS#1 keys. Alternatively, they can generate the key in PKCS#8 format from the start.

Checklist

  • I've signed all my commits
  • I ran just lint
  • I ran cargo test
  • I've checked the integration tests
  • I've followed the contribution guidelines
  • I'm linking the issue being fixed by this PR (if any)

@qlrd qlrd force-pushed the fix-ssl-keypath branch from 4372092 to 78693d9 Compare March 12, 2025 20:29
@lucad70
Copy link
Contributor

lucad70 commented Mar 12, 2025

ACK.

That was a nice catch!

@Davidson-Souza
Copy link
Member

I think it's worth it remove the unwraps and do proper error handling/error-ing since we are touching this. Still exit, but in a clean shutdown with a decent log.

This way we won't have the same difficulty figuring out what is wrong

@lucad70
Copy link
Contributor

lucad70 commented Mar 12, 2025

I can help you with that if needed @qlrd

@qlrd qlrd force-pushed the fix-ssl-keypath branch 3 times, most recently from 72fd1f1 to 5f0e82c Compare March 14, 2025 01:04
@qlrd
Copy link
Contributor Author

qlrd commented Mar 14, 2025

@lucad70, just learned about .map_err but don't know if this is a correct approach.

Can you review?

@qlrd qlrd force-pushed the fix-ssl-keypath branch from 5f0e82c to 0344aa3 Compare March 14, 2025 01:16
@qlrd
Copy link
Contributor Author

qlrd commented Mar 14, 2025 •
edited
Loading

Changed some documentation to florestad --help. What you guys think?

$> florestad --help
   ...
   ...
      --ssl-cert-path <PATH>
          Path to the SSL certificate file (defaults to <data-dir>/ssl/cert.pem).

          The user should create a PKCS#8 based one with openssl. For example:

          openssl req -x509 -new -key key.pem -out cert.pem -days 365 -subj "/CN=localhost"

      --ssl-key-path <PATH>
          Path to the SSL private key file (defaults to <data-dir>/ssl/key.pem).

          The user should create a PKCS#8 based one with openssl. For example:

          openssl genpkey -algorithm RSA -out key.pem -pkeyopt rsa_keygen_bits:2048
    ...

@qlrd qlrd force-pushed the fix-ssl-keypath branch from 0344aa3 to 7ff8d74 Compare March 14, 2025 12:57
lucad70
lucad70 reviewed Mar 14, 2025
View reviewed changes
@qlrd qlrd force-pushed the fix-ssl-keypath branch from 7ff8d74 to 6bb5937 Compare March 14, 2025 21:09
@qlrd qlrd changed the title fix load the SSL key as certificate and add documentation about geneate keys fix load the SSL key as certificate and add documentation about generate keys Mar 15, 2025
Davidson-Souza
Davidson-Souza approved these changes Mar 15, 2025
View reviewed changes
Copy link
Member

@Davidson-Souza Davidson-Souza left a comment

Choose a reason for hiding this comment

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

utACK 6bb5937

@qlrd qlrd requested a review from lucad70 March 16, 2025 15:09
lucad70
lucad70 suggested changes Mar 17, 2025
View reviewed changes
Copy link
Contributor

@lucad70 lucad70 left a comment

Choose a reason for hiding this comment

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

The error handling has improved massively, but you need to update the method call to correctly display it to the user. I suggest on letting it pass even with error and fixing the no_ssl logic on a followup PR as it would touch other parts of the code. However, other more experienced devs might add their own thoughts to it.

@qlrd qlrd force-pushed the fix-ssl-keypath branch from 6bb5937 to a4f1878 Compare March 17, 2025 16:32
@qlrd qlrd requested review from Davidson-Souza and lucad70 March 17, 2025 17:25
lucad70
lucad70 reviewed Mar 17, 2025
View reviewed changes
@qlrd qlrd force-pushed the fix-ssl-keypath branch 2 times, most recently from 4bedee3 to eaaafdb Compare March 18, 2025 00:48
@qlrd
Fix load the SSL private key as certificate and add documentation abo…
b094d7b 
…ut generate keys.

The florestad/src/florestad.rs::create_tls_config function was loading
the SSL key as certificate and not as a key due to the the variable
`key_path` wrongly loading it as `self.config.ssl_cert_path` (and not as
`self.config.ssl_key_path`). So, this commit:

* Add a docstring in both florestad.rs and cli.rs with the steps to
create the SSL private key and the certificate file;

* correctly load the SSL private key file as a private key;

* agnostically build paths to those files in a way that both UNIX and
Windows will accept;

* make a proper error handling/error-ing;

* warn the user when florestad cannot load TLS (files does not exist,
bad format, corrupted, or any reason of failure);

* fixed the "always log but may never listen" bug when not find a valid
SSL files (Davidson diff applied).

remove diff
@qlrd qlrd force-pushed the fix-ssl-keypath branch from eaaafdb to b094d7b Compare March 18, 2025 01:00
@lucad70
Copy link
Contributor

lucad70 commented Mar 18, 2025

ACK b094d7b

Davidson-Souza
Davidson-Souza approved these changes Mar 18, 2025
View reviewed changes
Copy link
Member

@Davidson-Souza Davidson-Souza left a comment

Choose a reason for hiding this comment

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

ACK b094d7b

@Davidson-Souza Davidson-Souza merged commit 5a3f471 into vinteumorg:master Mar 18, 2025
8 checks passed
@qlrd qlrd mentioned this pull request Mar 21, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Davidson-Souza Davidson-Souza Davidson-Souza approved these changes

+1 more reviewer

@lucad70 lucad70 lucad70 requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug] florestad load the SSL key as certificate and documentation has a lack of info how to generate it correctly

3 participants

@qlrd @lucad70 @Davidson-Souza