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

Update backend dependencies #1120

Merged
merged 10 commits into from
Feb 23, 2024
Merged

Update backend dependencies #1120

merged 10 commits into from
Feb 23, 2024

Conversation

LukasKalbertodt
Copy link
Member

@LukasKalbertodt LukasKalbertodt commented Feb 23, 2024 •
edited
Loading

This finally updates all backend dependencies, including some big breaking changes. The most time was required by the hyper 1.0 update and the update of all the crypto related libraries.

I don't think a normal code review is all too useful. At most a quick look. But more importantly I think is testing several things. Ideally this PR shouldn't be merged before we have checked all of this:

  • Opencast sync with HTTP
  • Opencast sync with HTTPS
  • from_login_credentials = "opencast" HTTP
  • from_login_credentials = "opencast" HTTPS
  • db.tls_mode = "on"
  • db.tls_mode = "without-verify-cert"
  • db.tls_mode = "off"
  • The Tokio update bumps the effective required libc version. Check if it still works on test deployment.
  • Auth callback with HTTP
  • Auth callback with HTTPS
  • Login callback with HTTP
  • Login callback with HTTPS
  • JWT still works (generation of key, generation of JWTs)
  • Tobira sessions still work

@LukasKalbertodt
Run cargo update
c05b505
@LukasKalbertodt
Update cookie from 0.17 -> 0.18
8aeb42e
@LukasKalbertodt
Update Tokio from 1.28 to 1.36
286c623 
Fixes elan-ev#934
@LukasKalbertodt
Update deadpool* dependencies
0a434e6
@LukasKalbertodt
Update crypto-related crates
3c13718 
This was a fairly annoying update as it contains many breaking changes.
And `ring` is annoying as usual by not providing a changelog. Now that
rustls supports other crypto providers, maybe we can get rid of ring
at some point.

To be honest, I have not tested all code paths. I think it should work,
but we should probably try loading a certificate and using TLS with the
DB before the next release... or ideally before this PR is merged.
@LukasKalbertodt
Update procfs and prometheus-client
4a37bf8 
Both without breaking changes affecting us, luckily.
@LukasKalbertodt
Bump minimum required Rust version
0cf6da8 
The dependency upgrades required that.
@LukasKalbertodt
Prepare hyper update
f78a60d
@LukasKalbertodt
Update hyper to 1.0
6682be0 
Wow, that was a painful update. There is still some work to be done:
- At two places we create a client and `unwrap`. That client needs to
  be created only once during startup.
- Syncing and all other HTTP client using things need to be tested.
- Unix socket have to be tested (including proper output)
@LukasKalbertodt
Put HttpsClient in context to avoid recreating one for every request
a6322b1
@LukasKalbertodt LukasKalbertodt added the changelog:dev Changes primarily for developers label Feb 23, 2024
@github-actions github-actions bot temporarily deployed to test-deployment-pr1120 February 23, 2024 09:07 Destroyed
@LukasKalbertodt
Copy link
Member Author

@geichelberger Do you have an easy way to test the two remaining test cases, i.e. tls_mode = "on" and "without-verify-cert"? I don't have a PostgreSQL server with TLS at hand. I can surely somehow test this with one of our clients, but if this happens to be easy for you, could you quickly test that this PR still works in those modes? If it's also some effort for you, don't worry, I can find a way to test it myself.

@geichelberger
Copy link
Contributor

I have nothing ready to use; why not just enable TLS for the Postgres container?

@LukasKalbertodt
Copy link
Member Author

"just" -> do you know something I don't? From Googling around it doesn't seem that straight forward to do that.

@geichelberger
Copy link
Contributor

podman run --rm -p 5432:5432 -e POSTGRES_PASSWORD=password postgres:16 -c ssl=on -c ssl_cert_file=/etc/ssl/certs/ssl-cert-snakeoil.pem -c ssl_key_file=/etc/ssl/private/ssl-cert-snakeoil.key

@geichelberger
Copy link
Contributor

psql "sslmode=require" -h 127.0.0.1 -U postgres

If SSL is not enabled you get an error message: psql: error: connection to server at "127.0.0.1", port 5432 failed: server does not support SSL, but SSL was required

@LukasKalbertodt
Copy link
Member Author

Oh it has a dummy cert built-in, cool. Thanks for the command.

Now the without-verify-cert case is tested and works. Only the "on" case is remaining but I'm very very sure that will work as well. In "without-verify-cert", the communication is already via TLS. The cert verification shouldn't have changed. So with this, I would say it's ready to be reviewed/merged.

@geichelberger
Copy link
Contributor

The most crucial test case for on is to check if it fails with an untrusted certificate :), but it should work to add the cert to your trust store.

@LukasKalbertodt
Copy link
Member Author

Good point but yeah, I did indeed test that and it errored :D So that's a good sign.

owi92
owi92 approved these changes Feb 23, 2024
View reviewed changes
Copy link
Member

@owi92 owi92 left a comment

Choose a reason for hiding this comment

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

Following your discussion, it seems this is ready to be merged. Also did some quick testing myself and didn't find any issues.

@owi92 owi92 merged commit 97d28b2 into elan-ev:master Feb 23, 2024
4 checks passed
@LukasKalbertodt LukasKalbertodt deleted the update-backend-dependencies branch February 23, 2024 12:14
@LukasKalbertodt LukasKalbertodt added the changelog:admin Changes primarily for admins label Feb 23, 2024
@LukasKalbertodt LukasKalbertodt mentioned this pull request Mar 4, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@owi92 owi92 owi92 approved these changes

Assignees
No one assigned
Labels
changelog:admin Changes primarily for admins changelog:dev Changes primarily for developers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LukasKalbertodt @geichelberger @owi92