Skip to content

Commit ddcf8ff

Browse files
committed
URL signing secret: extra polish and clarificatoin
1 parent 5706157 commit ddcf8ff

15 files changed

Lines changed: 174 additions & 54 deletions

File tree

conf/keycloak/docker-compose-dev.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ services:
3131
DATAVERSE_FEATURE_API_BEARER_AUTH_USE_BUILTIN_USER_ON_ID_MATCH: "1"
3232
DATAVERSE_MAIL_SYSTEM_EMAIL: "dataverse@localhost"
3333
DATAVERSE_MAIL_MTA_HOST: "smtp"
34+
DATAVERSE_API_SIGNING_SECRET: "dev-only-signing-secret-change-me"
3435
DATAVERSE_AUTH_OIDC_ENABLED: "1"
3536
DATAVERSE_AUTH_OIDC_CLIENT_ID: test
3637
DATAVERSE_AUTH_OIDC_CLIENT_SECRET: 94XHrfNRwXsjqTqApRrwWmhDLDHpIYV8

doc/release-notes/fix-url-signing-special-characters.md

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,12 @@ they did before 6.10 keep working unchanged, signatures are computed the same wa
1919
regression, and URLs containing special characters validate again. No client-side changes are
2020
required.
2121

22+
Note that, as before 6.10, the `url` submitted to `/api/admin/requestSignedUrl` (and any URL consumed
23+
as a signed URL) must be in its URL-decoded form: the signature is computed over the URL exactly as
24+
provided, but the server validates it against the URL-decoded request. Passing a percent-encoded URL
25+
(e.g. `persistentId=doi%3A10.5072%2FFK2%2FABC` instead of `persistentId=doi:10.5072/FK2/ABC`) and then
26+
using it verbatim will therefore fail validation.
27+
2228
### A signing secret is now required for signed URLs
2329

2430
Separately from the fix above, Dataverse no longer falls back to a weak signing key when
@@ -31,9 +37,13 @@ URLs are signed with a key based on a user's API token:
3137
guestbook-response download endpoints under `/api/access` (`datafile/{id}`, `datafiles/{ids}`,
3238
`dataset/{id}` and `dataset/{id}/versions/{versionId}`) - return an error instead of issuing a
3339
weakly-signed URL.
34-
- Signed callbacks built internally (external tool launches, Globus transfers) are sent unsigned,
35-
with a warning logged, rather than weakly signed; such unsigned URLs cannot be used to access
36-
draft datasets or restricted files.
40+
- External tool launches send their callback unsigned (with a warning logged) rather than weakly
41+
signed. Such an unsigned callback only allows anonymous access to public data; it cannot be used to
42+
access draft datasets or restricted files.
43+
- Globus transfers are effectively disabled without a signing secret: the callbacks the
44+
dataverse-globus app relies on require an authenticated, signed request, so Globus upload (and
45+
download of restricted or unpublished files) fails with an authorization error. Installations that
46+
use Globus must set `dataverse.api.signing-secret`.
3747
- The permissions-history CSV download links on the permission management pages are not offered
3848
(a warning is logged).
3949

doc/sphinx-guides/source/api/native-api.rst

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9079,7 +9079,7 @@ Signed URLs were developed to support External Tools but may be useful in other
90799079
This API call allows a Dataverse superUser to generate a signed URL for such scenarios.
90809080
The JSON input parameter required is an object with the following keys:
90819081
9082-
- ``url`` - the exact URL to sign, including api version number and all query parameters
9082+
- ``url`` - the exact URL to sign, including api version number and all query parameters. Provide it in its URL-decoded form (for example ``persistentId=doi:10.5072/FK2/ABC``, not ``persistentId=doi%3A10.5072%2FFK2%2FABC``): the signature is computed over the URL exactly as provided, but Dataverse validates it against the URL-decoded request, so a percent-encoded URL used verbatim will fail validation.
90839083
- ``timeOut`` - how long in minutes the signature should be valid for, default is 10 minutes
90849084
- ``httpMethod`` - which HTTP method is required, default is GET
90859085
- ``user`` - the user identifier for the account associated with this signature, the default is the superuser making the call. The API call will succeed/fail based on whether the specified user has the required permissions.

doc/sphinx-guides/source/developers/globus-api.rst

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ Note that while Dataverse will not add files that violate the size or quota rule
7878
they intend to transfer before submitting a transfer request to Globus.
7979

8080
The getDatasetMetadata and getFileListing URLs are just signed versions of the standard Dataset metadata and file listing API calls. The other two are Globus specific.
81-
Note that these URLs are only signed when a non-empty :ref:`dataverse.api.signing-secret` is configured on the Dataverse installation; without it, they are sent unsigned (a warning is logged) and cannot be used to access draft datasets or restricted files.
81+
Note that these URLs are only signed when a non-empty :ref:`dataverse.api.signing-secret` is configured on the Dataverse installation. Without it the callbacks are sent unsigned (a warning is logged), and because the Globus callback endpoints (``globusUploadParameters``, ``requestGlobusUploadPaths``, ``addGlobusFiles``, and the download equivalents for restricted or unpublished files) require an authenticated, signed request, the Globus transfer flow fails with an authorization error; only anonymous download of public files can proceed. In practice, Globus transfer support requires ``dataverse.api.signing-secret`` to be set.
8282

8383
If called for a dataset using a store that is configured with a remote Globus endpoint(s), the return response is similar but the response includes a
8484
the "managed" parameter will be false, the "endpoint" parameter is replaced with a JSON array of "referenceEndpointsWithPaths" and the

docker/compose/demo/compose.yml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@ services:
1616
DATAVERSE_FEATURE_API_BEARER_AUTH: "1"
1717
DATAVERSE_MAIL_SYSTEM_EMAIL: "Demo Dataverse <dataverse@example.org>"
1818
DATAVERSE_MAIL_MTA_HOST: "smtp"
19+
# Required for signed URLs (external tools, Globus transfers, signed guestbook downloads,
20+
# rdm-integration). Change this to a private value; see the Configuration Guide.
21+
DATAVERSE_API_SIGNING_SECRET: "demo-only-signing-secret-change-me"
1922
JVM_ARGS: -Ddataverse.files.storage-driver-id=file1
2023
-Ddataverse.files.file1.type=file
2124
-Ddataverse.files.file1.label=Filesystem

src/main/java/edu/harvard/iq/dataverse/api/Access.java

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -548,6 +548,13 @@ private Map<Long, DataFile> getDatafilesMap(DataverseRequest req, String fileIds
548548
}
549549

550550
private Response returnSignedUrl(ContainerRequestContext crc, UriInfo uriInfo, User user, String id, String gbrids) {
551+
// Record the guestbook-response id for the side-effect-only callers
552+
// (datafileBundleWithGuestbookResponse, postDownloadDatafiles) that discard the Response we
553+
// return and then read this property to drive the actual download. This must happen even when
554+
// no signing secret is configured and we bail out below, otherwise those downloads lose the
555+
// guestbook response that processDatafileWithGuestbookResponse just saved (leading to a
556+
// spurious "guestbookResponseMissing" 400 or a duplicate guestbook/MakeDataCount entry).
557+
crc.setProperty("gbrids", gbrids);
551558
// The signed-URL endpoints already check this before any guestbook side effects (see
552559
// requireSigningSecretForSignedUrl). This check protects the callers that invoke
553560
// processDatafileWithGuestbookResponse only for its guestbook side effects and discard this
@@ -587,7 +594,6 @@ private Response returnSignedUrl(ContainerRequestContext crc, UriInfo uriInfo, U
587594
builder.replaceQueryParam("gbrids", gbrids);
588595
}
589596
builder.replaceQueryParam("persistentId", null); // remove this as a parm and add the id to the path
590-
crc.setProperty("gbrids", gbrids);
591597
String baseUrlEncoded = builder.build().toString();
592598
String baseUrl = URLDecoder.decode(baseUrlEncoded, StandardCharsets.UTF_8);
593599
baseUrl = baseUrl.replace(":persistentId", id);

src/main/java/edu/harvard/iq/dataverse/api/Admin.java

Lines changed: 27 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -2459,30 +2459,37 @@ public Response getSignedUrl(@Context ContainerRequestContext crc, JsonObject ur
24592459
"Requesting signed URLs requires a signing secret to be configured. Please set the dataverse.api.signing-secret JVM option.");
24602460
}
24612461

2462-
String userId = urlInfo.getString("user");
2463-
String key=null;
2464-
if (userId != null) {
2465-
AuthenticatedUser user = authSvc.getAuthenticatedUser(userId);
2466-
// If a user param was sent, we sign the URL for them, otherwise on behalf of
2467-
// the superuser who made this api call
2468-
if (user != null) {
2469-
ApiToken apiToken = authSvc.findApiTokenByUser(user);
2470-
if (apiToken != null && !apiToken.isExpired() && !apiToken.isDisabled()) {
2471-
key = apiToken.getTokenString();
2472-
}
2473-
} else {
2474-
userId = superuser.getUserIdentifier();
2475-
// We ~know this exists - the superuser just used it and it was unexpired/not
2476-
// disabled. (ToDo - if we want this to work with workflow tokens (or as a
2477-
// signed URL), we should do more checking as for the user above))
2478-
key = authSvc.findApiTokenByUser(superuser).getTokenString();
2462+
// "url" is required; "user" defaults to the superuser making the call (see the docs for this
2463+
// endpoint). Use the defaulted accessors: JsonObject.getString(name) throws NullPointerException
2464+
// when the key is absent.
2465+
String baseUrl = urlInfo.getString("url", null);
2466+
if (baseUrl == null) {
2467+
return error(Response.Status.BAD_REQUEST, "Required parameter 'url' is missing.");
2468+
}
2469+
String userId = urlInfo.getString("user", null);
2470+
2471+
String key = null;
2472+
AuthenticatedUser signingUser = (userId != null) ? authSvc.getAuthenticatedUser(userId) : null;
2473+
if (signingUser != null) {
2474+
// A known user was requested: sign the URL for them.
2475+
ApiToken apiToken = authSvc.findApiTokenByUser(signingUser);
2476+
if (apiToken != null && !apiToken.isExpired() && !apiToken.isDisabled()) {
2477+
key = apiToken.getTokenString();
24792478
}
2480-
if (key == null) {
2481-
return error(Response.Status.CONFLICT, "Do not have a valid user with apiToken");
2479+
} else {
2480+
// No user param, or an unknown one: sign on behalf of the superuser who made this API call.
2481+
userId = superuser.getUserIdentifier();
2482+
// The superuser just authenticated, but that does not guarantee an API token exists (e.g.
2483+
// bearer-token or session auth), so null-check rather than dereference blindly.
2484+
ApiToken apiToken = authSvc.findApiTokenByUser(superuser);
2485+
if (apiToken != null) {
2486+
key = apiToken.getTokenString();
24822487
}
24832488
}
2489+
if (key == null) {
2490+
return error(Response.Status.CONFLICT, "Do not have a valid user with apiToken");
2491+
}
24842492

2485-
String baseUrl = urlInfo.getString("url");
24862493
int timeout = urlInfo.getInt(URLTokenUtil.TIMEOUT, 10);
24872494
String method = urlInfo.getString(URLTokenUtil.HTTP_METHOD, "GET");
24882495

src/main/java/edu/harvard/iq/dataverse/api/auth/SignedUrlAuthMechanism.java

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
import edu.harvard.iq.dataverse.authorization.users.*;
55
import edu.harvard.iq.dataverse.privateurl.PrivateUrl;
66
import edu.harvard.iq.dataverse.privateurl.PrivateUrlServiceBean;
7-
import edu.harvard.iq.dataverse.settings.JvmSettings;
87
import edu.harvard.iq.dataverse.util.UrlSignerUtil;
98

109
import jakarta.inject.Inject;
@@ -52,6 +51,14 @@ private String getSignedUrlRequestParameter(ContainerRequestContext containerReq
5251

5352
private User getAuthenticatedUserFromSignedUrl(ContainerRequestContext containerRequestContext) {
5453
User user = null;
54+
// Without a signing secret we never issue signed URLs (signUrlWithApiKey refuses on the sign
55+
// side), so we must not accept them here either. Otherwise a bare API token - or, for a guest,
56+
// the public request URL - would be enough to forge a URL whose signature validates against the
57+
// "" + token key computed below. Reject so findUserFromRequest returns the standard 401.
58+
if (!UrlSignerUtil.isSigningSecretConfigured()) {
59+
logger.warning("Rejecting signed URL authentication: no signing secret configured (dataverse.api.signing-secret).");
60+
return null;
61+
}
5562
// The signedUrl contains a param telling which user this is supposed to be for.
5663
// We don't trust this. So we look up that user, and get their API key, and use
5764
// that as a secret in validating the signedURL. If the signature can't be
@@ -60,6 +67,11 @@ private User getAuthenticatedUserFromSignedUrl(ContainerRequestContext container
6067
// If User is Guest we can return a generic guest user with key made from URI
6168
UriInfo uriInfo = containerRequestContext.getUriInfo();
6269
String userId = uriInfo.getQueryParameters().getFirst(SIGNED_URL_USER);
70+
if (userId == null) {
71+
// A token param was present (that is why this mechanism ran) but no user param: this can
72+
// never be a URL we signed, and dereferencing userId below would throw a NullPointerException.
73+
return null;
74+
}
6375
User targetUser = null;
6476
ApiToken userApiToken = null;
6577
if (userId.equalsIgnoreCase("guest")) {
@@ -90,7 +102,7 @@ private User getAuthenticatedUserFromSignedUrl(ContainerRequestContext container
90102
}
91103

92104
String requestMethod = containerRequestContext.getMethod();
93-
String signedUrlSigningKey = JvmSettings.API_SIGNING_SECRET.lookupOptional().orElse("") + userApiToken.getTokenString();
105+
String signedUrlSigningKey = UrlSignerUtil.getApiSigningKey(userApiToken.getTokenString());
94106
boolean isSignedUrlValid = UrlSignerUtil.isValidUrl(signedUrl, userId, requestMethod, signedUrlSigningKey);
95107
if (isSignedUrlValid) {
96108
user = targetUser;

src/main/java/edu/harvard/iq/dataverse/externaltools/ExternalToolHandler.java

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -110,12 +110,8 @@ public String handleRequest(boolean preview) {
110110
+ externalTool.getId();
111111
}
112112
if (apiToken != null) {
113-
if (UrlSignerUtil.isSigningSecretConfigured()) {
114-
callback = UrlSignerUtil.signUrlWithApiKey(callback, 5, apiToken.getAuthenticatedUser().getUserIdentifier(),
115-
HttpMethod.GET, apiToken.getTokenString());
116-
} else {
117-
logger.warning("Cannot sign external tool callback: no signing secret configured (dataverse.api.signing-secret). Sending an unsigned callback.");
118-
}
113+
callback = UrlSignerUtil.trySignUrlWithApiKey(callback, 5, apiToken.getAuthenticatedUser().getUserIdentifier(),
114+
HttpMethod.GET, apiToken.getTokenString(), "external tool callback");
119115
}
120116
paramsString= "?callback=" + Base64.getEncoder().encodeToString(StringUtils.getBytesUtf8(callback));
121117
if (getLocaleCode() != null) {

src/main/java/edu/harvard/iq/dataverse/globus/GlobusServiceBean.java

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -779,11 +779,12 @@ public String getGlobusAppUrlForDataset(Dataset d, boolean upload, List<DataFile
779779
if (apiToken == null) {
780780
// Shouldn't happen
781781
logger.warning("Unable to get api token for user: " + user.getIdentifier());
782-
} else if (UrlSignerUtil.isSigningSecretConfigured()) {
783-
callback = UrlSignerUtil.signUrlWithApiKey(callback, 5, apiToken.getAuthenticatedUser().getUserIdentifier(),
784-
HttpMethod.GET, apiToken.getTokenString());
785782
} else {
786-
logger.warning("Cannot sign Globus callback: no signing secret configured (dataverse.api.signing-secret). Sending an unsigned callback.");
783+
// Note: without a signing secret the callback is sent unsigned, which the dataverse-globus
784+
// app cannot use - the Globus upload/download callback endpoints require an authenticated,
785+
// signed request. Globus transfers therefore require dataverse.api.signing-secret.
786+
callback = UrlSignerUtil.trySignUrlWithApiKey(callback, 5, apiToken.getAuthenticatedUser().getUserIdentifier(),
787+
HttpMethod.GET, apiToken.getTokenString(), "Globus callback");
787788
}
788789
appUrl = appUrl + "&callback=" + Base64.getEncoder().encodeToString(StringUtils.getBytesUtf8(callback));
789790

0 commit comments

Comments
 (0)