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.

Sign up for GitHub

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

Jump to bottom

ARSN-387: check for forwarded proto header #2210

Open
wants to merge 3 commits into
base: development/7.10
Choose a base branch
from
Open

ARSN-387: check for forwarded proto header #2210

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9e3e723
ARSN-387: check for forwarded proto header
Jan 19, 2024
8edd2f1
ARSN-387: update test
Jan 19, 2024
0466eb4
fixup: update test
Feb 9, 2024
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
2 changes: 1 addition & 1 deletion lib/policyEvaluator/utils/conditions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ export function findConditionKey(
case 'aws:referer': return headers.referer;
// aws:SecureTransport – Used to check whether the request was sent
// using SSL (see Boolean Condition Operators).
case 'aws:SecureTransport': return requestContext.getSslEnabled() ? 'true' : 'false';
case 'aws:SecureTransport': return headers?.['x-forwarded-proto'] === 'https' ? 'true' : 'false';
// aws:SourceArn – Used check the source of the request,
// using the ARN of the source. N/A here.
case 'aws:SourceArn': return undefined;
Expand Down
2 changes: 1 addition & 1 deletion lib/policyEvaluator/utils/variables.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ function findVariable(variable: string, requestContext: RequestContext): string
// aws:SecureTransport is boolean value that represents whether the
// request was sent using SSL
map.set('aws:SecureTransport',
requestContext.getSslEnabled() ? 'true' : 'false');
headers?.['x-forwarded-proto'] === 'https' ? 'true' : 'false');
Copy link
Contributor

Choose a reason for hiding this comment

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

Headers shouldn't be blindly trusted, as they could be forged if not explicitly overwritten by the http frontend - and since I'm not sure artesca is in scope for this PR, the forward port will automatically introduce the vulnerability.

This should be disabled by default and wrapped and enabled with explicit configuration, similar to how client IP conditions are handled.
https://github.com/scality/Arsenal/blob/development/7.10/lib/policyEvaluator/requestUtils.ts

Copy link
Author

@KazToozs KazToozs Mar 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Supposing I revert this Arsenal change, would something like this in Cloudserver work according to you?:
scality/cloudserver@a66f293

function _checkBucketPolicyConditions(request, conditions, log) {
    const ip = request ? requestUtils.getClientIp(request, config) : undefined;
    if (!conditions) {
        return true;
    }
    const viaProxy = config.requests.viaProxy;

    const sslEnabled = (viaProxy ?
        request.headers['x-forwarded-proto'] === 'https' : request.connection.encrypted);
    // build request context from the request!
    const requestContext = new RequestContext(request.headers, request.query,
        request.bucketName, request.objectKey, ip,
        sslEnabled, request.resourceType, 's3', null, null,
        null, null, null, null, null, null, null, null, null,
        request.objectLockRetentionDays);
    return evaluators.meetConditions(requestContext, conditions, log);
}

Copy link
Contributor

@rachedbenmustapha rachedbenmustapha Mar 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

x-forwarded-proto should be a configurable, set by the same deployment infrastructure than the one that sets up the reverse proxy, so that no misguided cross-component assumptions are made. So for S3C that's Federation, and for artesca it's shared between artesca-installer and zenko-operator.

The reason I insist that we make it configurable and disabled by default even for RING is that I'm not sure 100% of customers go through the nginx we ship, some of them might use their own LB which may or may not overwrite x-forwarded-proto (or use a different header altogether). Or some might in the future and we don't want any surprises.

// aws:SourceIp is requester's IP address, for use with IP address
// conditions
map.set('aws:SourceIp', requestContext.getRequesterIp());
Expand Down
8 changes: 6 additions & 2 deletions tests/unit/policyEvaluator.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -906,7 +906,9 @@ describe('policyEvaluator', () => {
() => {
policy.Statement.Condition = { Bool:
{ 'aws:SecureTransport': 'true' } };
const rcModifiers = { _sslEnabled: false };
const rcModifiers = { _headers: {
'x-forwarded-proto': 'http',
} };
check(requestContext, rcModifiers, policy, 'Neutral');
});

Expand All @@ -915,7 +917,9 @@ describe('policyEvaluator', () => {
() => {
policy.Statement.Condition = { Bool:
{ 'aws:SecureTransport': 'true' } };
const rcModifiers = { _sslEnabled: true };
const rcModifiers = { _headers: {
'x-forwarded-proto': 'https',
} };
check(requestContext, rcModifiers, policy, 'Allow');
});

Expand Down
Loading