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

Hide token/key from casual inspection #675

Merged
merged 6 commits into from
Feb 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions NAMESPACE
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# Generated by roxygen2: do not edit by hand

S3method(format,rsconnect_secret)
S3method(print,rsconnect_secret)
S3method(str,rsconnect_secret)
export(accountInfo)
export(accountUsage)
export(accounts)
Expand Down
4 changes: 4 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# rsconnect 0.8.30 (development version)

* `accountInfo()` and `servers()` now redacts sensitive information (secrets,
private keys, and certificates) to make it hard to accidentally reveal
such information in logs (#675).

* The logic used by `deployApp()` for determining whether you publish a
new update or update an existing app has been simplified. Now `appName`,
`account`, and `server` are used to find existing deployments. If none
Expand Down
73 changes: 61 additions & 12 deletions R/accounts.R
Original file line number Diff line number Diff line change
Expand Up @@ -272,23 +272,45 @@ setAccountInfo <- function(name, token, secret,
if (is.null(accountId))
stop("Unable to determine account id for account named '", name, "'")

registerCloudTokenSecret(
serverName = serverInfo$name,
accountName = name,
userId = userId,
token = token,
secret = secret,
)
invisible()
}

registerCloudTokenSecret <- function(serverName,
accountName,
userId,
accountId,
token,
secret) {
# get the path to the config file
configFile <- accountConfigFile(name, serverInfo$name)
dir.create(dirname(configFile), recursive = TRUE, showWarnings = FALSE)
path <- accountConfigFile(accountName, serverName)
dir.create(dirname(path), recursive = TRUE, showWarnings = FALSE)

# write the user info
write.dcf(list(name = name,
userId = userId,
accountId = accountId,
token = token,
secret = secret,
server = serverInfo$name),
configFile,
width = 100)
write.dcf(
list(
name = accountName,
userId = userId,
accountId = accountName,
token = token,
secret = secret,
server = serverName
),
path,
width = 100
)

# set restrictive permissions on it if possible
if (identical(.Platform$OS.type, "unix"))
Sys.chmod(configFile, mode = "0600")
Sys.chmod(path, mode = "0600")

path
}

#' @rdname accounts
Expand All @@ -303,8 +325,13 @@ accountInfo <- function(name = NULL, server = NULL) {
info <- as.list(accountDcf)
# remove all whitespace from private key
if (!is.null(info$private_key)) {
info$private_key <- gsub("[[:space:]]", "", info$private_key)
info$private_key <- secret(gsub("[[:space:]]", "", info$private_key))
Copy link
Contributor

Choose a reason for hiding this comment

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

Does rsconnect::accounts need the same adjustment?

This adjusts responses from the package API and should be included in NEWS.md.

Copy link
Member Author

Choose a reason for hiding this comment

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

accounts() just includes name and server — did you mean servers()?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably, yes!

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure that the certificates field in servers() is actually secret, but it is very long, so redacting it is a service to the user.

}

if (!is.null(info$secret)) {
info$secret <- secret(info$secret)
}

info
}

Expand Down Expand Up @@ -487,3 +514,25 @@ accountInfoFromHostUrl <- function(hostUrl) {
return(accountInfo(name = as.character(account[1, "name"]),
server = server))
}


secret <- function(x) {
stopifnot(is.character(x))
structure(x, class = "rsconnect_secret")
}

#' @export
format.rsconnect_secret <- function(x, ...) {
paste0(substr(x, 1, 6), "... (redacted)")
}

#' @export
print.rsconnect_secret <- function(x, ...) {
print(format(x))
invisible(x)
}

#' @export
str.rsconnect_secret <- function(object, ...) {
cat(" ", format(object), "\n", sep = "")
}
8 changes: 5 additions & 3 deletions R/servers.R
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,11 @@ servers <- function(local = FALSE) {
info
})
locals <- do.call(rbind, parsed)
locals <- as.data.frame(locals, stringsAsFactors = FALSE)
if (local) {
locals
out <- locals
} else {
serversList <- rbind(
out <- rbind(
locals,
as.data.frame(shinyappsServerInfo(), stringsAsFactors = FALSE),
as.data.frame(cloudServerInfo(), stringsAsFactors = FALSE))
Expand All @@ -75,8 +76,9 @@ servers <- function(local = FALSE) {
as.data.frame(cloudServerInfo("rstudio.cloud"), stringsAsFactors = FALSE)
)
}
serversList
}
out$certificate <- secret(out$certificate)
out
}

serverConfigDir <- function() {
Expand Down
20 changes: 20 additions & 0 deletions tests/testthat/_snaps/accounts.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# secrets are hidden from casual inspection

Code
accountInfo("john")$private_key
Output
[1] "THISIS... (redacted)"
Code
accountInfo("susan")$secret
Output
[1] "THIS I... (redacted)"
Code
str(accountInfo("john"))
Output
List of 5
$ username : chr "john"
$ accountId : chr "userId"
$ token : chr "token"
$ server : chr "example.com"
$ private_key: THISIS... (redacted)

10 changes: 10 additions & 0 deletions tests/testthat/_snaps/servers.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# servers() redacts the certificate

Code
servers()
Output
name url certificate
1 cert_test_a https://localhost:4567/ -----B... (redacted)
2 shinyapps.io https://api.shinyapps.io/v1 Amazon... (redacted)
3 posit.cloud https://api.shinyapps.io/v1 Amazon... (redacted)

21 changes: 21 additions & 0 deletions tests/testthat/test-accounts.R
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,24 @@ test_that("All hosted product names are identified as cloud", {
expect_true(isCloudServer("posit.cloud"))
expect_false(isCloudServer("connect.internal"))
})

test_that("secrets are hidden from casual inspection", {
local_temp_config()
registerUserToken("example.com", "john", "userId", "token", "THIS IS A SECRET")
registerCloudTokenSecret(
"shinyapps.io",
"susan",
"userId",
"accountId",
"token",
"THIS IS A SECRET"
)

expect_snapshot({
accountInfo("john")$private_key
accountInfo("susan")$secret

str(accountInfo("john"))
})

})
12 changes: 12 additions & 0 deletions tests/testthat/test-servers.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
test_that("servers() redacts the certificate", {
local_temp_config()

# add a server with a sample certificate
addServer(
url = "https://localhost:4567/",
name = "cert_test_a",
certificate = test_path("certs/sample.crt"),
quiet = TRUE
)
expect_snapshot(servers())
})