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

Enable TLS protocol when cert is provided and port number is 443 #113

Merged
merged 3 commits into from
Mar 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 README.md
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ In order to deploy both the stacks the user needs to provide a set of required a
| customRoleArn | Optional | string | User provided IAM role arn to be used as ec2 instance profile. `-c customRoleArn=arn:aws:iam::<AWS_ACCOUNT_ID>:role/<ROLE_NAME>`|
| customConfigFiles | Optional | string | You can provide an entire config file to be overwritten or added to OpenSearch and OpenSearch Dashboards. Pass string in the form of JSON with key as local path to the config file to read from and value as file on the server to overwrite/add. Note that the values in the JSON needs to have prefix of `opensearch` or `opensearch-dashboards`. Example: `-c customConfigFiles='{"opensearch-config/config.yml": "opensearch/config/opensearch-security/config.yml", "opensearch-config/role_mapping.yml":"opensearch/config/opensearch-security/roles_mapping.yml", "/roles.yml": "opensearch/config/opensearch-security/roles.yml"}'` |
| enableMonitoring | Optional | boolean | Boolean flag to enable monitoring and alarms for Infra Stack. See [InfraStackMonitoring class](./lib/monitoring/alarms.ts) for more details. Defaults to false e.g., `--context enableMonitoring=true` |
| certificateArn | Optional | string | Add ACM certificate to the listener. e.g., `--context certificateArn=arn:1234`|
| certificateArn | Optional | string | Add ACM certificate to the any listener (OpenSearch or OpenSearch-Dashboards) whose port is mapped to 443. e.g., `--context certificateArn=arn:1234`|
| mapOpensearchPortTo | Optional | integer | Load balancer port number to map to OpenSearch. e.g., `--context mapOpensearchPortTo=8440` Defaults to 80 when security is disabled and 443 when security is enabled |
| mapOpensearchDashboardsPortTo | Optional | integer | Load balancer port number to map to OpenSearch-Dashboards. e.g., `--context mapOpensearchDashboardsPortTo=443` Always defaults to 8443 |

Expand Down
46 changes: 32 additions & 14 deletions lib/infra/infra-stack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@

constructor(scope: Stack, id: string, props: InfraProps) {
super(scope, id, props);
let opensearchListener: NetworkListener;
let dashboardsListener: NetworkListener;
let managerAsgCapacity: number;
let dataAsgCapacity: number;
Expand Down Expand Up @@ -402,6 +403,8 @@
});

const opensearchPortMap = `${props?.mapOpensearchPortTo ?? scope.node.tryGetContext('mapOpensearchPortTo')}`;
const opensearchDashboardsPortMap = `${props?.mapOpensearchDashboardsPortTo ?? scope.node.tryGetContext('mapOpensearchDashboardsPortTo')}`;

if (opensearchPortMap === 'undefined') {
if (!this.securityDisabled && !this.minDistribution) {
this.opensearchPortMapping = 443;
Expand All @@ -412,32 +415,47 @@
this.opensearchPortMapping = parseInt(opensearchPortMap, 10);
}

const opensearchListener = nlb.addListener('opensearch', {
port: this.opensearchPortMapping,
protocol: Protocol.TCP,
});
if (!this.securityDisabled && !this.minDistribution) {
if (certificateArn !== 'undefined') {
opensearchListener.addCertificates('cert', [ListenerCertificate.fromArn(certificateArn)]);
}
}

const opensearchDashboardsPortMap = `${props?.mapOpensearchDashboardsPortTo ?? scope.node.tryGetContext('mapOpensearchDashboardsPortTo')}`;
if (opensearchDashboardsPortMap === 'undefined') {
this.opensearchDashboardsPortMapping = 8443;
} else {
this.opensearchDashboardsPortMapping = parseInt(opensearchDashboardsPortMap, 10);
}

if (this.dashboardsUrl !== 'undefined') {
dashboardsListener = nlb.addListener('dashboards', {
port: this.opensearchDashboardsPortMapping,
if (this.opensearchPortMapping === this.opensearchDashboardsPortMapping) {
throw new Error('OpenSearch and OpenSearch-Dashboards cannot be mapped to the same port! Please provide different port numbers.'
+ ` Current mapping is OpenSearch:${this.opensearchPortMapping} OpenSearch-Dashboards:${this.opensearchDashboardsPortMapping}`);
}

if (!this.securityDisabled && !this.minDistribution && this.opensearchPortMapping === 443 && certificateArn !== 'undefined') {
Copy link
Member

Choose a reason for hiding this comment

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

So it is possible for either os or osd to bind to 443, but not both?

In that case what if someone wants to get TLS on both os and osd?

Copy link
Member Author

Choose a reason for hiding this comment

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

For now we have enforced that TLS can only be enabled for 443 port. It can be any OS or OSD and certificate ARN is compulsory.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For now we decided to go with HTTPS port with TLS. Can surely be extended in future for all ports.

opensearchListener = nlb.addListener('opensearch', {

Check warning on line 430 in lib/infra/infra-stack.ts

View check run for this annotation

Codecov / codecov/patch

lib/infra/infra-stack.ts#L430

Added line #L430 was not covered by tests
port: this.opensearchPortMapping,
protocol: Protocol.TLS,
certificates: [ListenerCertificate.fromArn(certificateArn)],
});
} else {
opensearchListener = nlb.addListener('opensearch', {
port: this.opensearchPortMapping,
protocol: Protocol.TCP,
});
}

if (this.dashboardsUrl !== 'undefined') {
if (!this.securityDisabled && !this.minDistribution && this.opensearchDashboardsPortMapping === 443 && certificateArn !== 'undefined') {
dashboardsListener = nlb.addListener('dashboards', {
port: this.opensearchDashboardsPortMapping,
protocol: Protocol.TLS,
certificates: [ListenerCertificate.fromArn(certificateArn)],
});
} else {
dashboardsListener = nlb.addListener('dashboards', {
port: this.opensearchDashboardsPortMapping,
protocol: Protocol.TCP,
});
}
}

if (this.singleNodeCluster) {
console.log('Single node value is true, creating single node configurations');

Check warning on line 458 in lib/infra/infra-stack.ts

View workflow job for this annotation

GitHub Actions / build

Unexpected console statement
singleNodeInstance = new Instance(this, 'single-node-instance', {
vpc: props.vpc,
instanceType: singleNodeInstanceType,
Expand Down
4 changes: 2 additions & 2 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@opensearch-project/opensearch-cluster-cdk",
"version": "1.2.0",
"version": "1.2.1",
"bin": {
"cdk_v2": "bin/app.js"
},
Expand Down
85 changes: 83 additions & 2 deletions test/opensearch-cluster-cdk.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -895,19 +895,58 @@ test('Test certificate addition and port mapping', () => {
infraTemplate.hasResourceProperties('AWS::ElasticLoadBalancingV2::Listener', {
Port: 8440,
Protocol: 'TCP',
});
infraTemplate.hasResourceProperties('AWS::ElasticLoadBalancingV2::Listener', {
Port: 443,
Protocol: 'TLS',
Certificates: [
{
CertificateArn: 'arn:1234',
},
],
});
});

test('Test default port mapping', () => {
const app = new App({
context: {
securityDisabled: false,
minDistribution: false,
distributionUrl: 'www.example.com',
cpuArch: 'x64',
singleNodeCluster: false,
dashboardsUrl: 'www.example.com',
distVersion: '1.0.0',
serverAccessType: 'ipv4',
restrictServerAccessTo: 'all',
},
});

// WHEN
const networkStack = new NetworkStack(app, 'opensearch-network-stack', {
env: { account: 'test-account', region: 'us-east-1' },
});

// @ts-ignore
const infraStack = new InfraStack(app, 'opensearch-infra-stack', {
vpc: networkStack.vpc,
securityGroup: networkStack.osSecurityGroup,
env: { account: 'test-account', region: 'us-east-1' },
});

// THEN
const infraTemplate = Template.fromStack(infraStack);
infraTemplate.hasResourceProperties('AWS::ElasticLoadBalancingV2::Listener', {
Port: 443,
Protocol: 'TCP',
});
infraTemplate.hasResourceProperties('AWS::ElasticLoadBalancingV2::Listener', {
Port: 8443,
Protocol: 'TCP',
});
});

test('Test default port mapping', () => {
test('Ignore cert and TLS protocol if none of the ports map to 443', () => {
const app = new App({
context: {
securityDisabled: false,
Expand All @@ -919,6 +958,9 @@ test('Test default port mapping', () => {
distVersion: '1.0.0',
serverAccessType: 'ipv4',
restrictServerAccessTo: 'all',
certificateArn: 'arn:1234',
mapOpensearchPortTo: '8440',
mapOpensearchDashboardsPortTo: '8443',
},
});

Expand All @@ -937,11 +979,50 @@ test('Test default port mapping', () => {
// THEN
const infraTemplate = Template.fromStack(infraStack);
infraTemplate.hasResourceProperties('AWS::ElasticLoadBalancingV2::Listener', {
Port: 443,
Port: 8440,
Protocol: 'TCP',
});
infraTemplate.hasResourceProperties('AWS::ElasticLoadBalancingV2::Listener', {
Port: 8443,
Protocol: 'TCP',
});
});

test('Throw error on duplicate ports', () => {
const app = new App({
context: {
securityDisabled: false,
minDistribution: false,
distributionUrl: 'www.example.com',
cpuArch: 'x64',
singleNodeCluster: false,
dashboardsUrl: 'www.example.com',
distVersion: '1.0.0',
serverAccessType: 'ipv4',
restrictServerAccessTo: 'all',
certificateArn: 'arn:1234',
mapOpensearchPortTo: '8443',
},
});
// WHEN
try {
// WHEN
const networkStack = new NetworkStack(app, 'opensearch-network-stack', {
env: { account: 'test-account', region: 'us-east-1' },
});

// @ts-ignore
const infraStack = new InfraStack(app, 'opensearch-infra-stack', {
vpc: networkStack.vpc,
securityGroup: networkStack.osSecurityGroup,
env: { account: 'test-account', region: 'us-east-1' },
});

// eslint-disable-next-line no-undef
fail('Expected an error to be thrown');
} catch (error) {
expect(error).toBeInstanceOf(Error);
// @ts-ignore
expect(error.message).toEqual('OpenSearch and OpenSearch-Dashboards cannot be mapped to the same port! Please provide different port numbers. Current mapping is OpenSearch:8443 OpenSearch-Dashboards:8443');
}
});
Loading