Skip to content
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.

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

feat(rpc): add https support #1861

Merged
merged 3 commits into from
Jun 20, 2023
Merged

feat(rpc): add https support #1861

merged 3 commits into from
Jun 20, 2023

Conversation

shamardy
Copy link
Collaborator

@shamardy shamardy commented Jun 13, 2023

This PR adds a new mm2 config parameter https. When this parameter is set to true, mm2 automatically generates a self-signed certificate internally if no certificate and private key file is found in either the specified paths (MM_CERT_PATH and MM_CERT_KEY_PATH) as defined in the environment variables or if no cert.pem and key.pem files are found in the same directory as the compiled mm2 binary (similar to how coins file works).

For the self-signed certificate, the default SANs (Subject Alternative Names) used are localhost and 127.0.0.1, these are set if alt_names is not specified in mm2 config. alt_names takes an array of strings specifying the desired SANs.

After certificates are loaded, the mm2 service/server is then executed in HTTPS mode, enabling secure communication between the client and server using Transport Layer Security (TLS). This ensures that all requests and responses are encrypted, providing enhanced security for the API.

When https is not set, it defaults to false and the mm2 service is executed in HTTP as it was before this PR to maintain backward compatibility for clients relying on it.

Please note that disabling certificate verification/validation is required by the client for requests to work in https mode if self-signed certificates where used.

New dependencies:
pem v1.1.1
rcgen v0.10.0
yasna v0.5.2

dependency update:
rustls-pemfile 1.0.0 -> 1.0.2

Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

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

Great work! First review iteration from my side:

mm2src/mm2_main/src/rpc.rs Outdated Show resolved Hide resolved
mm2src/mm2_main/src/rpc.rs Outdated Show resolved Hide resolved
mm2src/mm2_main/src/rpc.rs Outdated Show resolved Hide resolved
mm2src/mm2_main/src/rpc.rs Outdated Show resolved Hide resolved
mm2src/mm2_net/src/native_tls/acceptor.rs Outdated Show resolved Hide resolved
@shamardy shamardy added in progress Changes will be made from the author under review and removed under review in progress Changes will be made from the author labels Jun 14, 2023
onur-ozkan
onur-ozkan previously approved these changes Jun 14, 2023
Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

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

This looks good to me.

The last suggestion I would say for this PR, please have 3 seperated commits:

1: Port acceptor and builders with containing ported from https://github.com/rustls/hyper-rustls/tree/286e1fa57ff5cac99994fab355f91c3454d6d83d this commit description

2- Do the changes on these ported modules

3- All the rest of the work.

In the worst-case scenario, by having incremental changes starting from https://github.com/rustls/hyper-rustls/tree/286e1fa57ff5cac99994fab355f91c3454d6d83d in the ported source code, the developers will be able to easily understand and quickly identify the patches we have applied. It is important to keep these commits separate and preserve them in our commit history. Otherwise, it would be exhausting to compare and match the correct version/commit in the source code with the corresponding modules in our ported code.

@shamardy
Copy link
Collaborator Author

shamardy commented Jun 15, 2023

The last suggestion I would say for this PR, please have 3 seperated commits
In the worst-case scenario, by having incremental changes starting from https://github.com/rustls/hyper-rustls/tree/286e1fa57ff5cac99994fab355f91c3454d6d83d in the ported source code, the developers will be able to easily understand and quickly identify the patches we have applied.

That's a really neat idea, these should be the guidelines for any ported code to mm2. I will do it this way and force push the changes. I will also add these suggestions to contribution guidelines later.

Copy link
Member

@borngraced borngraced left a comment

Choose a reason for hiding this comment

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

Great work and welldone!
my review

mm2src/mm2_main/src/rpc.rs Outdated Show resolved Hide resolved
mm2src/mm2_main/src/rpc.rs Outdated Show resolved Hide resolved
mm2src/mm2_net/src/native_tls/builder.rs Show resolved Hide resolved
@shamardy shamardy added in progress Changes will be made from the author under review and removed under review in progress Changes will be made from the author labels Jun 17, 2023
@onur-ozkan
Copy link
Member

You would need to rebase the PR and have proper commit history since we will not squash this one on merge

- add the required deps for native_tls to mm2_net/Cargo.toml
- expose TlsStream and add remote_addr to it
- format the ported code
@shamardy
Copy link
Collaborator Author

You would need to rebase the PR and have proper commit history since we will not squash this one on merge

Done, now I have 1 commit for the porting, 1 for the changes of the ported code and 1 for the rest of work in mm2.

@borngraced can you please do a final review if you have more comments?

Copy link
Member

@borngraced borngraced left a comment

Choose a reason for hiding this comment

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

Great work! well done 🙏🏼

@shamardy shamardy merged commit c00dd35 into dev Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants