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.

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

fix: validate certificate generation #283

Merged
merged 29 commits into from
Aug 13, 2024
Merged

fix: validate certificate generation #283

merged 29 commits into from
Aug 13, 2024

Conversation

Manuthor
Copy link
Contributor

@Manuthor Manuthor commented Jul 24, 2024 •
edited
Loading

  • Use Validate KMIP Operation to verify certificates generated by KMS server
    • validate also self-signed certificate
  • Regenerate test certificates to include a CRL linked to package.cosmian.com
  • Simplify Validate flow by first ordering certificates list (then certificate chain signature and revocation status can be be verified in cascade)
  • Do not verify revocation status across all chain CRLs (only parent or attached certificate CRL distribution point)
  • Add small cache for CRLs (avoiding too many external requests)
  • Use a workaround for the hyper Incomplete error by not using client certificate authentication in tests (Incomplete error being quite hard to reproduce in local - not in CI!)
  • Logs: externalize RUST_LOG variable declaration to ease tests-debugging
  • Upgrade cargo deps to last patch versions and fix cargo deny

@Manuthor Manuthor force-pushed the more_certs_validate branch 6 times, most recently from 4c764e6 to e1852a2 Compare July 24, 2024 15:26
@Manuthor Manuthor force-pushed the more_certs_validate branch from 5f49314 to 70053a5 Compare August 2, 2024 08:10
@Manuthor Manuthor requested review from Adamk93 and bgrieder August 6, 2024 20:36
@Manuthor Manuthor marked this pull request as ready for review August 6, 2024 20:37
Manuthor and others added 20 commits August 9, 2024 08:08
@Manuthor
fix: pedantic clippy lints
317dbb5
@Manuthor
fix: validate certificate generation
cadb495
@Manuthor
fix: try disable hyper idle pool
8e4dcb6
@Manuthor
fix: add retry on KMS rest client
dae261f
@Manuthor
fix: mitigate hyper error: IncompleteMessage: connection closed befor…
ad85c2b 
…e message completed (#285)

* fix: try fix hyper problem

* chore: log kms rest client error

* chore: log kms rest client error

* fix: re-add small delay after a hyper incompletemessage error

* fix: revert test cli changes

* fix: revert test_server change

* fix: small fix in certificate indexing

* fix: try new timeout on reqwest

* fix: control RUST_LOG from variable env.

* chore: force sleep before retry

* fix: use default features of reqwest

* fix: sync reqwest features between client and server

* chore: bump reqwest to 0.12

* fix: set default timeout on server. Remove delay in fetching CRL

* fix: try to block async thread on client request

* fix: keep retry on failure on KMS rest client
@Manuthor
ci: fix tests on certificate validation
7df5637
@Manuthor
chore: revert CI change, remove filter on tests certify
a6d3dee
@Manuthor
test: connection pool
3e34024
@Manuthor
chore: use std::RwLock instead of tokio::RwLock
5f4d7ee
@Manuthor
chore: bump sqlx version
932f1dd
@Manuthor
chore: remove http crate and reuse actix-web:http crate
3f8a3de
@Manuthor
chore: ignore temporarly 2 tests on server
c5c28cf
@Manuthor
test: set max idle connection on validate operation
70f70c7
@Manuthor
test: remove RwLock on client
a4f185e
@Manuthor
test: use rustls-tls for server
7616b98
@Manuthor
chore: downgrade hyper-util to 0.1.5
923a85e
@Manuthor
test: disabling cert auth in test_server
d0f8d29
@Manuthor
fix: remove logs in test - not required for now
99df61e
@Manuthor
chore: revert downgrade of hyper-util + remove useless join_all
0685fb1
@Manuthor
docs: add doc validation
e84a0d9
@Manuthor Manuthor force-pushed the more_certs_validate branch from 947f356 to 591a5cc Compare August 10, 2024 11:35
@Manuthor Manuthor mentioned this pull request Aug 13, 2024
Closed
Adamk93
Adamk93 reviewed Aug 13, 2024
View reviewed changes
crate/cli/src/main.rs
Copy link
Contributor

Choose a reason for hiding this comment

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

here you added a real log right?

Copy link
Contributor Author

@Manuthor Manuthor Aug 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

log_init initializes the logger if:

  • RUST_LOG variable is sourced
  • or if the input argument default_value is filled
    In that case, the CLI will have logs if RUST_LOG is sourced.
    Does it seem right?

Adamk93
Adamk93 approved these changes Aug 13, 2024
View reviewed changes
Copy link
Contributor

@Adamk93 Adamk93 left a comment

Choose a reason for hiding this comment

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

I have no particular comments. I put some questions, but really, curiosities. Otherwise it seems perfect to me.

crate/cli/test_data/certificates/csr/extensions.ext
Copy link
Contributor

Choose a reason for hiding this comment

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

cool now the tests uses a CRL stored in https://package.cosmian.com/kms/crl_tests/intermediate.crl.pem . Before it was just a mock url right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

before, all those certificates were not validated by ckms (and then the CRL URL was never checked)

crate/client/src/kms_rest_client.rs
.default_headers(headers)
.tcp_keepalive(Duration::from_secs(60))
Copy link
Contributor

Choose a reason for hiding this comment

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

was this a problem before?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, the timeout passed from 5 to 60 sec :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem hyper is occurring (from time to time) when client authentication by certificate is enabled on the Rust Client.
I believe the 60s delay has no impact on this problem.
Currently, I've disabled the certificate authentication wherever it is not needed (this is only a workaround). It could be fixed properly using reqwest and rustls-tls instead of native-tls.

@Manuthor Manuthor merged commit cd379b7 into develop Aug 13, 2024
35 checks passed
@Manuthor Manuthor deleted the more_certs_validate branch August 13, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Adamk93 Adamk93 Adamk93 approved these changes

@bgrieder bgrieder Awaiting requested review from bgrieder

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Manuthor @Adamk93