sha1 token signing workaround

[jonkeane]

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.

[59] "Running the tests in �tests/testthat.R� failed."
[60] "Last 13 lines of output:"
[61] " 'test-appMetadata-quarto.R:54:3', 'test-appMetadata-quarto.R:64:3',"
[62] " 'test-appMetadata-quarto.R:74:3', 'test-appMetadata-quarto.R:90:3',"
[63] " 'test-appMetadata-quarto.R:115:3', 'test-appMetadata-quarto.R:134:3',"
[64] " 'test-appMetadata.R:14:3', 'test-deploySite.R:2:3'"
[65] " "
[66] " �� Failed tests ����������������������������������������������������������������"
[67] " �� Failure ('test-bundlePackage.R:145:3'): package_record works ����������������"
[68] " windows1251Record$Author (`actual`) not equal to \"Се�гей ��ин\" (`expected`)."
[69] " "
[70]" `actual`: \"\\xd1\\xe5\\xf0\\xe3\\xe5\\xe9 \\xc1\\xf0\\xe8\\xed\""
[71] " `expected`: \"Се�гей ��ин\" "
[72] " "
[73] " [ FAIL 1 | WARN 3 | SKIP 145 | PASS 631 ]"

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).

[jonkeane]

I'm sure there's a better way to do this, but I wanted to get feedback on this quicker

[aronatkins]

This is equivalent:

pem <- openssl::write_pem(private_key)
pem_lines <- readLines(textConnection(pem))
pki_key <- PKI::PKI.load.key(pem_lines, format = "PEM")

[jonkeane]

PKI doesn't support non-RSA keys. If I understand correctly this is totally fine because we only generate RSA keys.

Was the use of ed25519 just because it was shorter?

I also committed these changes separately to be able to pull that and see if tests past there.

[aronatkins]

I'm guessing so, yes (shorter).

I would also be fine if the fake-key were a constant in-code rather than a file living alongside the tests. You could store it in PEM format, which has the start/end markers and (generally) shorter lines.

[jonkeane]

Thanks, I pulled the PEM format into the test. It's not the prettiest, but the sidecar file also isn't either.

[aronatkins]

I feel like we should have a test proving that this implementation mirrors the old implementation. Right now, we're using the textual snapshot as a way of convincing ourselves, but it would be nice to have something that's enforced by code, as well.

It might mean refactoring signatureHeaders so we can pass the canonicalRequest into some signRequest helper, but that would let us test equivalence beyond the signature check. We could confirm that the "new" implementation returns the same signature as the old approach, which used openssl.

signRequestPrivateKey_old <- function(private_key, canonicalRequest) {
  rawsig <- openssl::signature_create(charToRaw(canonicalRequest), key = private_key)
  openssl::base64_encode(rawsig)
}

signRequestPrivateKey_new <- function(private_key, canonicalRequest) {
  pem <- openssl::write_pem(private_key)
  key <- readLines(textConnection(pem))
  pki_key <- PKI::PKI.load.key(key, format = "PEM")
  digested <- digest::digest(charToRaw(canonicalRequest), "sha1", serialize = FALSE, raw = TRUE)
  rawsig <- PKI::PKI.sign(key = pki_key, digest = digested)
  openssl::base64_encode(rawsig)
}

[aronatkins]

This is equivalent:

pem <- openssl::write_pem(private_key)
pem_lines <- readLines(textConnection(pem))
pki_key <- PKI::PKI.load.key(pem_lines, format = "PEM")

[aronatkins]

I'm guessing so, yes (shorter).

I would also be fine if the fake-key were a constant in-code rather than a file living alongside the tests. You could store it in PEM format, which has the start/end markers and (generally) shorter lines.

[aronatkins]

Forgot to mention: This needs NEWS.

[jonkeane]

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.

[jonkeane]

This is ready for review again

[aronatkins]

Thanks for that additional test.

[jonkeane]

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.

[aronatkins]

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.

[venpopov]

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: #1073