ARSN-387: check for forwarded proto header

[KazToozs]

This fix is for the aws:secureTransport condition, related to the TSKB.

With load balancers in front, the check for SSL in the request must be done on the x-forwarded-proto header.

Tests have been updated accordingly.

The other necessary change for this condition to work correctly is for the nginx config to properly pass this header:

proxy_set_header X-Forwarded-Proto $scheme;

Edit:
Green CS and Vault builds:
https://github.com/scality/Vault/pull/2151
scality/cloudserver#5546

[bert-e]

Hello kaztoozs,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Status report is not available.

[bert-e]

Request integration branches

Waiting for integration branch creation to be requested by the user.

To request integration branches, please comment on this pull request with the following command:

/create_integration_branches

Alternatively, the /approve and /create_pull_requests commands will automatically
create the integration branches.

[KazToozs]

/create_integration_branches

[bert-e]

Integration data created

I have created the integration data for the additional destination branches.

• this pull request will merge bugfix/ARSN-387-ssl-check-fix into
  development/7.10
• w/7.70/bugfix/ARSN-387-ssl-check-fix will be merged into development/7.70
• w/8.1/bugfix/ARSN-387-ssl-check-fix will be merged into development/8.1

The following branches will NOT be impacted:

• development/6.4
• development/7.4

You can set option create_pull_requests if you need me to create
integration pull requests in addition to integration branches, with:

@bert-e create_pull_requests

The following options are set: create_integration_branches

[bert-e]

Waiting for approval

The following approvals are needed before I can proceed with the merge:

• the author

• 2 peers

The following options are set: create_integration_branches

[rachedbenmustapha]

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

[KazToozs]

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);
}

[rachedbenmustapha]

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.

[fredmnl]

I may be wrong but I'm seeing that the getSslEnabled function has become dead code, it's not called anywhere anymore. We should either look into removing that if it's not needed, or perhaps the checking of the forwarded header should actually be put in that function. I'm not sure which one it is because I'm not sure what I'm looking at with regards to the requestcontext object.