Skip to content

Commit

Permalink
Support detecting the http scheme for policy and RQ evaluation
Browse files Browse the repository at this point in the history
- Bucket policies evaluation is also updated to not compute
  the request context for each statement, as it will remain
  the same for a given request anyway.
- The isSecure flag is computed every time we need to create
  request contexts to send to the IAM service.

Issue: CLDSRV-602
  • Loading branch information
williamlardier committed Jan 10, 2025
1 parent 2330879 commit e90f02e
Show file tree
Hide file tree
Showing 8 changed files with 37 additions and 27 deletions.
33 changes: 16 additions & 17 deletions lib/api/apiUtils/authorization/permissionChecks.js
Original file line number Diff line number Diff line change
Expand Up @@ -255,28 +255,17 @@ function _checkBucketPolicyActions(requestType, actions, log) {
return evaluators.isActionApplicable(mappedAction, actions, log);
}

function _checkBucketPolicyResources(request, resource, log) {
if (!request || (Array.isArray(resource) && resource.length === 0)) {
function _checkBucketPolicyResources(requestContext, resource, log) {
if (!requestContext || (Array.isArray(resource) && resource.length === 0)) {
return true;
}
// build request context from the request!
const requestContext = new RequestContext(request.headers, request.query,
request.bucketName, request.objectKey, null,
request.connection.encrypted, request.resourceType, 's3');
return evaluators.isResourceApplicable(requestContext, resource, log);
}

function _checkBucketPolicyConditions(request, conditions, log) {
const ip = request ? requestUtils.getClientIp(request, config) : undefined;
if (!conditions) {
function _checkBucketPolicyConditions(requestContext, conditions, log) {
if (!requestContext || !conditions) {
return true;
}
// build request context from the request!
const requestContext = new RequestContext(request.headers, request.query,
request.bucketName, request.objectKey, ip,
request.connection.encrypted, request.resourceType, 's3', null, null,
null, null, null, null, null, null, null, null, null,
request.objectLockRetentionDays);
return evaluators.meetConditions(requestContext, conditions, log);
}

Expand Down Expand Up @@ -332,12 +321,22 @@ function checkBucketPolicy(policy, requestType, canonicalID, arn, bucketOwner, l
permission = 'allow';
}
let copiedStatement = JSON.parse(JSON.stringify(policy.Statement));

const ip = request ? requestUtils.getClientIp(request, config) : undefined;
const isSecure = request ? requestUtils.getHttpProtocolSecurity(request, config) : undefined;
const requestContext = request ? new RequestContext(request.headers, request.query,
request.bucketName, request.objectKey, ip,
isSecure, request.resourceType, 's3', null, null,
null, null, null, null, null, null, null, null, null,
request.objectLockRetentionDays) : undefined;

while (copiedStatement.length > 0) {
const s = copiedStatement[0];

const principalMatch = _checkPrincipals(canonicalID, arn, s.Principal);
const actionMatch = _checkBucketPolicyActions(requestType, s.Action, log);
const resourceMatch = _checkBucketPolicyResources(request, s.Resource, log);
const conditionsMatch = _checkBucketPolicyConditions(request, s.Condition, log);
const resourceMatch = _checkBucketPolicyResources(requestContext, s.Resource, log);
const conditionsMatch = _checkBucketPolicyConditions(requestContext, s.Condition, log);

if (principalMatch && actionMatch && resourceMatch && conditionsMatch && s.Effect === 'Deny') {
// explicit deny trumps any allows, so return immediately
Expand Down
6 changes: 3 additions & 3 deletions lib/api/apiUtils/authorization/prepareRequestContexts.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,12 @@ function prepareRequestContexts(apiMethod, request, sourceBucket,
// check.

const ip = requestUtils.getClientIp(request, config);
const isSecure = requestUtils.getHttpProtocolSecurity(request, config);

function generateRequestContext(apiMethod) {
return new RequestContext(request.headers,
request.query, request.bucketName, request.objectKey,
ip, request.connection.encrypted,
apiMethod, 's3');
ip, isSecure, apiMethod, 's3');
}

if (apiMethod === 'bucketPut') {
Expand Down Expand Up @@ -84,7 +84,7 @@ function prepareRequestContexts(apiMethod, request, sourceBucket,
{ versionId: sourceVersionId });
const getRequestContext = new RequestContext(request.headers,
reqQuery, sourceBucket, sourceObject,
ip, request.connection.encrypted,
ip, isSecure,
objectGetAction, 's3');
const putRequestContext = generateRequestContext('objectPut');
requestContexts.push(getRequestContext, putRequestContext);
Expand Down
3 changes: 2 additions & 1 deletion lib/api/apiUtils/object/objectLockHelpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -283,14 +283,15 @@ function checkUserGovernanceBypass(request, authInfo, bucketMD, objectKey, log,

const authParams = auth.server.extractParams(request, log, 's3', request.query);
const ip = policies.requestUtils.getClientIp(request, config);
const isSecure = policies.requestUtils.getHttpProtocolSecurity(request, config);

Check warning on line 286 in lib/api/apiUtils/object/objectLockHelpers.js

View check run for this annotation

Codecov / codecov/patch

lib/api/apiUtils/object/objectLockHelpers.js#L286

Added line #L286 was not covered by tests
const requestContextParams = {
constantParams: {
headers: request.headers,
query: request.query,
generalResource: bucketMD.getName(),
specificResource: { key: objectKey },
requesterIp: ip,
sslEnabled: request.connection.encrypted,
sslEnabled: isSecure,
apiMethod: 'bypassGovernanceRetention',
awsService: 's3',
locationConstraint: bucketMD.getLocationConstraint(),
Expand Down
7 changes: 5 additions & 2 deletions lib/api/bucketPut.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,8 @@ function _parseXML(request, log, cb) {
});
}

function _buildConstantParams({ request, bucketName, authInfo, authParams, ip, locationConstraint, apiMethod }) {
function _buildConstantParams({
request, bucketName, authInfo, authParams, ip, isSecure, locationConstraint, apiMethod }) {
return {
constantParams: {
headers: request.headers,
Expand All @@ -121,7 +122,7 @@ function _buildConstantParams({ request, bucketName, authInfo, authParams, ip, l
key: '',
},
requesterIp: ip,
sslEnabled: request.connection.encrypted,
sslEnabled: isSecure,
awsService: 's3',
requesterInfo: authInfo,
signatureVersion: authParams.params.data.authType,
Expand Down Expand Up @@ -165,9 +166,11 @@ function _isAclProvided(headers) {

function authBucketPut(authParams, bucketName, locationConstraint, request, authInfo) {
const ip = requestUtils.getClientIp(request, config);
const isSecure = requestUtils.getHttpProtocolSecurity(request, config);
const baseParams = {
authParams,
ip,
isSecure,
bucketName,
request,
authInfo,
Expand Down
6 changes: 3 additions & 3 deletions lib/api/multiObjectDelete.js
Original file line number Diff line number Diff line change
Expand Up @@ -464,7 +464,6 @@ function getObjMetadataAndDelete(authInfo, canonicalID, request,
* @param {string} request.post - concatenation of request body
* @param {string} request.bucketName - parsed bucketName
* @param {string} request.socket.remoteAddress - requester IP
* @param {boolean} request.connection.encrypted - whether request was encrypted
* @param {object} log - Werelogs logger
* @param {function} callback - callback to server
* @return {undefined}
Expand All @@ -487,6 +486,8 @@ function multiObjectDelete(authInfo, request, log, callback) {
const inPlayInternal = [];
const bucketName = request.bucketName;
const canonicalID = authInfo.getCanonicalID();
const ip = requestUtils.getClientIp(request, config);
const isSecure = requestUtils.getHttpProtocolSecurity(request, config);

return async.waterfall([
function parseXML(next) {
Expand Down Expand Up @@ -546,14 +547,13 @@ function multiObjectDelete(authInfo, request, log, callback) {
// params on to this api
const authParams = auth.server.extractParams(request, log,
's3', request.query);
const ip = requestUtils.getClientIp(request, config);
const requestContextParams = {
constantParams: {
headers: request.headers,
query: request.query,
generalResource: request.bucketName,
requesterIp: ip,
sslEnabled: request.connection.encrypted,
sslEnabled: isSecure,
apiMethod: 'objectDelete',
awsService: 's3',
locationConstraint: null,
Expand Down
3 changes: 2 additions & 1 deletion lib/routes/routeMetadata.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,10 @@ function routeMetadata(clientIP, request, response, log) {
return responseJSONBody(errors.NotImplemented, null, response, log);
}
const ip = requestUtils.getClientIp(request, config);
const isSecure = requestUtils.getHttpProtocolSecurity(request, config);

Check warning on line 53 in lib/routes/routeMetadata.js

View check run for this annotation

Codecov / codecov/patch

lib/routes/routeMetadata.js#L53

Added line #L53 was not covered by tests
const requestContexts = [new RequestContext(request.headers, request.query,
request.generalResource, request.specificResource, ip,
request.connection.encrypted, request.resourceType, 'metadata')];
isSecure, request.resourceType, 'metadata')];
return waterfall([
next => auth.server.doAuth(request, log, (err, userInfo, authRes) => {
if (err) {
Expand Down
3 changes: 3 additions & 0 deletions tests/unit/api/deleteMarker.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ function _createMultiObjectDeleteRequest(numObjects) {
},
url: '/?delete',
query: { delete: '' },
socket: {
remoteAddress: '127.0.0.1',
},
actionImplicitDenies: false,
};
const xml = [];
Expand Down
3 changes: 3 additions & 0 deletions tests/unit/api/multiObjectDelete.js
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,9 @@ describe('multiObjectDelete function', () => {
'content-md5': crypto.createHash('md5').update(post, 'utf8').digest('base64')
},
post,
socket: {
remoteAddress: '127.0.0.1',
},
url: '/bucketname',
});
const authInfo = makeAuthInfo('123456');
Expand Down

0 comments on commit e90f02e

Please sign in to comment.