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

fix: Proxy and client cert authentication #1068

Merged
merged 10 commits into from
Mar 5, 2021
4 changes: 2 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

## Known Issues

-
-

## Compatibility Notes

Expand All @@ -22,7 +22,7 @@

## Improvements

-
- [http-agent] Fix client certificate authentication in conjunction with proxies - depends on [this PR](https://github.com/TooTallNate/node-https-proxy-agent/pull/111).

## Fixed Issues

Expand Down
21 changes: 14 additions & 7 deletions packages/core/src/connectivity/scp-cf/proxy-util.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { AgentOptions } from 'https';
import { HttpProxyAgent } from 'http-proxy-agent';
import { HttpsProxyAgent } from 'https-proxy-agent';
import { createLogger } from '@sap-cloud-sdk/util';
Expand Down Expand Up @@ -252,12 +253,15 @@ export function addProxyConfigurationInternet(destination: any): Destination {
/**
* Builds the http(s)-agent config. Note that the proxy agent type like http or https is determined by the destination RUL protocol.
* The protocol from the proxy is unrelated to this and in most cases http.
* All additional options are forwarded to tls.connect and net.connect see https://github.com/TooTallNate/node-https-proxy-agent#new-httpsproxyagentobject-options
*
* @param destination - Destination containing the proxy configurations
* @param options - Addiotional options for the agent
* @returns The http(s)-agent containing the proxy configuration
*/
export function proxyAgent(
destination: Destination
destination: Destination,
options?: AgentOptions
): HttpAgentConfig | HttpsAgentConfig {
const targetProtocol = getProtocolOrDefault(destination);
const proxyConfig = destination.proxyConfiguration;
Expand All @@ -266,18 +270,21 @@ export function proxyAgent(
throw new Error('Proxy config must not be undefined.');
}

const agentConfig = {
host: proxyConfig.host,
protocol: proxyConfig.protocol,
port: proxyConfig.port,
...options
FrankEssenberger marked this conversation as resolved.
Show resolved Hide resolved
};

switch (targetProtocol) {
case Protocol.HTTP:
return {
httpAgent: new HttpProxyAgent(
`${proxyConfig.protocol}://${proxyConfig.host}:${proxyConfig.port}`
)
httpAgent: new HttpProxyAgent(agentConfig)
};
case Protocol.HTTPS:
return {
httpsAgent: new HttpsProxyAgent(
`${proxyConfig.protocol}://${proxyConfig.host}:${proxyConfig.port}`
)
httpsAgent: new HttpsProxyAgent(agentConfig)
};
}
}
Expand Down
66 changes: 39 additions & 27 deletions packages/core/src/http-client/http-agent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,12 @@ export function getAgentConfig(
const agentType = destination.proxyConfiguration
? AgentType.PROXY
: AgentType.DEFAULT;
// eslint-disable-next-line @typescript-eslint/no-shadow
const certificateOptions = getCertificateOption(destination);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see shadow name. Could you please share a bit details?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for me I got a lint error that the variable in the if block below is shadowed - I do not understand that either.

if (agentType === AgentType.PROXY) {
return createProxyAgent(destination);
return createProxyAgent(destination,certificateOptions);
}
return createDefaultAgent(destination);
return createDefaultAgent(destination,certificateOptions);
FrankEssenberger marked this conversation as resolved.
Show resolved Hide resolved
}

enum AgentType {
Expand All @@ -41,21 +43,15 @@ enum AgentType {
}

function createProxyAgent(
destination: Destination
destination: Destination,
options: https.AgentOptions
): HttpAgentConfig | HttpsAgentConfig {
if (!destination.proxyConfiguration) {
throw new Error(
`The destination proxy configuration: ${destination.proxyConfiguration} is undefined.`
);
}

if (destination.isTrustingAllCertificates) {
logger.warn(
'The destination is configured to both use a proxy and to trust all certificates. This is currently not supported. The proxy configuration will be applied, but certificates will be validated.'
);
}

return proxyAgent(destination);
}
return proxyAgent(destination,options);
}

const trustAllOptions = (destination: Destination) => (
Expand All @@ -77,6 +73,31 @@ const certificateOptions = (destination: Destination) => (
}
return options;
};
/**
* The http agents (proxy and default) use node tls for the certificate handling. This method creates the options with the pfx and passphrase. *
* https://nodejs.org/api/tls.html#tls_tls_createsecurecontext_options
* @param destination - Destination object
* @returns options which can be used later by tls.createSecureContext() e.g. pfx and passphrase. or {} if protocol is not https or no client information are in the defintaion.
* @hidden
*/
function getCertificateOption(destination: Destination): Record<string, any> {
// http case: no certificate needed
if(getProtocolOrDefault(destination) === Protocol.HTTP){
if (destination.isTrustingAllCertificates) {
logger.warn('"isTrustingAllCertificates" is not available for HTTP.');
}
return {};
}
// https case: get certificate options
if (destination.isTrustingAllCertificates) {
FrankEssenberger marked this conversation as resolved.
Show resolved Hide resolved
logger.warn(
'"isTrustingAllCertificates" property in the provided destination is set to "true". This is highly discouraged in production.'
);
}

const options = trustAllOptions(destination)({});
return certificateOptions(destination)(options);
}

const supportedCertificateFormats = ['p12', 'pfx'];

Expand Down Expand Up @@ -116,26 +137,17 @@ function selectCertificate(destination): DestinationCertificate {

return certificate;
}

/*
See https://nodejs.org/api/https.html#https_https_createserver_options_requestlistener for details on the possible options
*/
function createDefaultAgent(
destination: Destination
destination: Destination,
options: https.AgentOptions
): HttpAgentConfig | HttpsAgentConfig {
if (getProtocolOrDefault(destination) === Protocol.HTTPS) {
if (destination.isTrustingAllCertificates) {
logger.warn(
'"isTrustingAllCertificates" property in the provided destination is set to "true". This is highly discouraged in production.'
);
}
let options = trustAllOptions(destination)({});
options = certificateOptions(destination)(options);

return { httpsAgent: new https.Agent(options) };
}

if (destination.isTrustingAllCertificates) {
logger.warn('"isTrustingAllCertificates" is not available for HTTP.');
}
return { httpAgent: new http.Agent() };
return { httpAgent: new http.Agent(options) };
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export interface UserAccessTokens {
export interface Systems {
s4: {
providerBasic: string;
providerClientCert: string;
providerOAuth2SAMLBearerAssertion: string;
subscriberBasic: string;
};
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {
executeHttpRequest,
fetchDestination,
fetchDestination, getDestination,
getService,
serviceToken,
userApprovedServiceToken,
Expand Down Expand Up @@ -125,6 +125,13 @@ describe('OAuth flows', () => {
expect(response.status).toBe(200);
}, 60000);

it('ClientCertificate: Fetches the certificate and uses it',async ()=>{
FrankEssenberger marked this conversation as resolved.
Show resolved Hide resolved
const destination = await getDestination('CC8-HTTP-CERT');
expect(destination!.certificates!.length).toBe(1);
const bps = await BusinessPartner.requestBuilder().getAll().top(5).execute(destination!);
expect(bps.length).toBeGreaterThan(0);
},10000);

xit('OAuth2UserTokenExchange: Subscriber destination and Subscriber Jwt', async () => {
const subscriberDestToken = await serviceToken('destination', {
userJwt: accessToken.subscriber
Expand Down