Closed jonkeane closed 3 months ago
Forgot to mention: This needs NEWS.
Thanks for the detailed review. I'm broadly onboard with the suggestions and will get to doing them shortly. But if this ends up blocking anything, (as always) I don't mind if you pushed to this branch to unblock it.
This is ready for review again
Thanks for that additional test.
Of course. Oh and I should have mentioned this FTR in the previous comment: I wanted to split that test out into it's own test_that
block (so it's clear what it's testing, but didn't want to also have to move the key elsewhere or duplicate it, so tacked it on as the least-disruptive option.
move the key elsewhere
No worries. You could create a helper function adjacent to the test_that
block, if that helps separate things. That helper could return something that resembles a "token", so you have both the public and private keys.
generateTokenFixed <- function() {
return list(
token = "TDECAFBAD",
public_key = "...",
private_key = "...",
)
}
test_that("thing one", {
token <- generateTokenFixed()
# check for stable signature (snapshot)
})
test_that("thing two", {
token <- generateTokenFixed()
# check for equivalent signature (openssl)
})
I don't think this adjustment is necessary, though. If we were to continue to expand the tests in this area, it's a natural next step.
Unfortunately (re)-introducing the PKI dependency in this PR makes it impossible to install rsconnect on MacOS when R is installed from homebrew rather than CRAN: https://github.com/rstudio/rsconnect/issues/1073
Seeing the use of
digest
for md5 hashes made me realize that we could do the same for the sha1 hashs (and PKI for signing).I tested this publishing to connect using an existing token from RStudio and it worked just fine.
I also ran the tests in docker running rockylinux9 with FIPS mode enabled (using the docker file described in #928 and running
R CMD build . && R CMD check .
) There was one test that fails, though this looks like an encoding issue unless I'm missing something.We should still move away from sha1 (possibly by moving to default to API keys, which already work and avoid this issue), but this PR buys us some time to do that work (both here and on the connect side).