-
Notifications
You must be signed in to change notification settings - Fork 126
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
Add AwsSigV4 signing functionality #279
Changes from 4 commits
d78ae1c
59bbbf4
caf4d74
7faaae1
6a67179
90a443d
e50e8fc
f58e21e
f1e5a16
93f347b
6d7b79c
8505f85
562d683
341df4f
f629fa1
c952ba0
4abf7ea
48e5b3a
e376ef4
1475bf0
52ab9ad
d31eb6f
f036b30
ef2b3c7
13d30de
b1308b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -152,6 +152,94 @@ async function search() { | |
search().catch(console.log); | ||
``` | ||
|
||
### With AWS SigV4 signing | ||
```javascript | ||
const endpoint = ""; // OpenSearch domain URL e.g. https://search-xxx.region.es.amazonaws.com | ||
const { Client } = require('@opensearch-project/opensearch'); | ||
const AwsV4Signer = require('@opensearch-project/opensearch/AwsV4Signer'); | ||
const { defaultProvider } = require("@aws-sdk/credential-provider-node"); | ||
|
||
async function getClient() { | ||
const credentials = await defaultProvider()(); | ||
var client = new Client({ | ||
...AwsV4Signer({ | ||
credentials: credentials, | ||
region: "us-west-2", | ||
}), | ||
node: endpoint, | ||
}); | ||
return client; | ||
} | ||
|
||
async function search() { | ||
|
||
var client = await getClient(); | ||
|
||
var index_name = "books-test-1"; | ||
var settings = { | ||
settings: { | ||
index: { | ||
number_of_shards: 4, | ||
number_of_replicas: 3, | ||
}, | ||
}, | ||
}; | ||
|
||
var response = await client.indices.create({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is identical to the samples above? Maybe we should break up the examples to specific sections (indexing, searching, AWS Sigv4 Signing...? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason why it lives in a separate code block is just because of the difference in the imports and the way the client is initialized. Because the signer needs credentials, it's an async call. Felt like maybe the two should be written separately. |
||
index: index_name, | ||
body: settings, | ||
}); | ||
|
||
console.log("Creating index:"); | ||
console.log(response.body); | ||
|
||
// Add a document to the index. | ||
var document = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All of the examples here look very similar to the already existing ones. I see that initializing the client is different, can we provide may be just one example here so we don't duplicate all the test data? Also, consider moving this to a USER_GUIDE since the number of examples is increasing, might make the README very long. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1, but I also think we can break this up in this PR or do it in another PR, so no hard ask from me. FYI I do like what they have done in the .NET client: https://github.com/opensearch-project/opensearch-net/blob/main/USER_GUIDE.md There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @VachaShah @dblock I've split the code snippets into a new USER_GUIDE. wdyt? |
||
title: "The Outsider", | ||
author: "Stephen King", | ||
year: "2018", | ||
genre: "Crime fiction", | ||
}; | ||
|
||
var id = "1"; | ||
|
||
var response = await client.index({ | ||
id: id, | ||
index: index_name, | ||
body: document, | ||
refresh: true, | ||
}); | ||
|
||
console.log("Adding document:"); | ||
console.log(response.body); | ||
|
||
var response = await client.bulk({ body: bulk_documents }); | ||
console.log(response.body); | ||
|
||
// Search for the document. | ||
var query = { | ||
query: { | ||
match: { | ||
title: { | ||
query: "The Outsider", | ||
}, | ||
}, | ||
}, | ||
}; | ||
|
||
var response = await client.search({ | ||
index: index_name, | ||
body: query, | ||
}); | ||
|
||
console.log("Search results:"); | ||
console.log(response.body.hits); | ||
} | ||
|
||
search().catch(console.log); | ||
``` | ||
|
||
|
||
## Project Resources | ||
|
||
- [Project Website](https://opensearch.org/) | ||
|
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
/* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
* | ||
* The OpenSearch Contributors require contributions made to | ||
* this file be licensed under the Apache-2.0 license or a | ||
* compatible open source license. | ||
* | ||
* Modifications Copyright OpenSearch Contributors. See | ||
* GitHub history for details. | ||
*/ | ||
|
||
import { Credentials } from '@aws-sdk/types'; | ||
import Connection from '../Connection'; | ||
import * as http from 'http'; | ||
|
||
interface AwsV4SignerOptions { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So technically this is "AwsSigv4". I think we should align with other implementations such as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, let's have them be consistent. I renamed everything. |
||
credentials: Credentials; | ||
region: string; | ||
} | ||
|
||
export interface AwsV4SignerResponse { | ||
Connection: Connection; | ||
buildSignedRequestObject(request: any): http.ClientRequestArgs; | ||
} | ||
|
||
export default function AwsV4Signer(opts: AwsV4SignerOptions): AwsV4SignerResponse; | ||
|
||
export {}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
/* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
* | ||
* The OpenSearch Contributors require contributions made to | ||
* this file be licensed under the Apache-2.0 license or a | ||
* compatible open source license. | ||
* | ||
* Modifications Copyright OpenSearch Contributors. See | ||
* GitHub history for details. | ||
*/ | ||
|
||
'use strict'; | ||
const Connection = require('../Connection'); | ||
const aws4 = require('aws4'); | ||
const { AwsV4SignerError } = require('./errors'); | ||
|
||
function AwsV4Signer(opts) { | ||
if (opts && (!opts.region || opts.region === null || opts.region === '')) { | ||
throw new AwsV4SignerError('Region cannot be empty'); | ||
} | ||
if (opts && (!opts.credentials || opts.credentials === null || opts.credentials === '')) { | ||
throw new AwsV4SignerError('Credentials cannot be empty'); | ||
} | ||
|
||
function buildSignedRequestObject(request = {}) { | ||
request.service = 'es'; | ||
request.region = opts.region; | ||
request.headers = request.headers || {}; | ||
request.headers['host'] = request.hostname; | ||
return aws4.sign(request, opts.credentials); | ||
} | ||
class AwsV4SignedConnection extends Connection { | ||
buildRequestObject(params) { | ||
const request = super.buildRequestObject(params); | ||
return buildSignedRequestObject(request); | ||
} | ||
} | ||
return { | ||
Connection: AwsV4SignedConnection, | ||
buildSignedRequestObject, | ||
}; | ||
} | ||
module.exports = AwsV4Signer; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
/* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
* | ||
* The OpenSearch Contributors require contributions made to | ||
* this file be licensed under the Apache-2.0 license or a | ||
* compatible open source license. | ||
* | ||
* Modifications Copyright OpenSearch Contributors. See | ||
* GitHub history for details. | ||
*/ | ||
import { OpenSearchClientError } from '../errors'; | ||
|
||
export declare class AwsV4SignerError extends OpenSearchClientError { | ||
name: string; | ||
message: string; | ||
data: any; | ||
constructor(message: string, data: any); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
/* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
* | ||
* The OpenSearch Contributors require contributions made to | ||
* this file be licensed under the Apache-2.0 license or a | ||
* compatible open source license. | ||
* | ||
* Modifications Copyright OpenSearch Contributors. See | ||
* GitHub history for details. | ||
*/ | ||
|
||
'use strict'; | ||
const { OpenSearchClientError } = require('../errors'); | ||
|
||
class AwsV4SignerError extends OpenSearchClientError { | ||
constructor(message, data) { | ||
super(message, data); | ||
Error.captureStackTrace(this, AwsV4SignerError); | ||
this.name = 'AwsV4SignerError'; | ||
this.message = message || 'AwsV4Signer Error'; | ||
this.data = data; | ||
} | ||
} | ||
|
||
module.exports = { | ||
AwsV4SignerError, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ | |
"require": "./index.js", | ||
"import": "./index.mjs" | ||
}, | ||
"./awsV4Signer": "./lib/aws/AwsV4Signer.js", | ||
"./": "./" | ||
}, | ||
"homepage": "https://www.opensearch.org/", | ||
|
@@ -77,6 +78,8 @@ | |
"xmlbuilder2": "^2.4.1" | ||
}, | ||
"dependencies": { | ||
"@aws-sdk/credential-provider-node": "^3.154.0", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So does this become a hard dependency for this package? Meaning if I am not doing AWS sigv4, am I still dragging this module in? Can it be avoided? Asking again because I'd like not to drag any vendor specific components in by default. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed this dependency like I mentioned below. We are still depending on the aws4 library which we cannot do away(unless we want to re-write all of that implementation) if we want to support signing natively in the client. |
||
"aws4": "^1.11.0", | ||
"debug": "^4.3.1", | ||
"hpagent": "^0.1.1", | ||
"ms": "^2.1.3", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
/* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
* | ||
* The OpenSearch Contributors require contributions made to | ||
* this file be licensed under the Apache-2.0 license or a | ||
* compatible open source license. | ||
* | ||
* Modifications Copyright OpenSearch Contributors. See | ||
* GitHub history for details. | ||
*/ | ||
|
||
/* | ||
* Licensed to Elasticsearch B.V. under one or more contributor | ||
harshavamsi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright | ||
* ownership. Elasticsearch B.V. licenses this file to you under | ||
* the Apache License, Version 2.0 (the "License"); you may | ||
* not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
import { expectType } from 'tsd'; | ||
const { v4: uuidv4 } = require('uuid'); | ||
import AwsV4Signer, { AwsV4SignerResponse } from '../../lib/aws/AwsV4Signer'; | ||
|
||
const mockCreds = { | ||
accessKeyId: uuidv4(), | ||
secretAccessKey: uuidv4(), | ||
}; | ||
|
||
const mockRegion = 'us-west-2'; | ||
|
||
{ | ||
const AwsV4SignerOptions = { credentials: mockCreds, region: mockRegion }; | ||
|
||
const auth = AwsV4Signer(AwsV4SignerOptions); | ||
|
||
expectType<AwsV4SignerResponse>(auth); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
/* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
* | ||
* The OpenSearch Contributors require contributions made to | ||
* this file be licensed under the Apache-2.0 license or a | ||
* compatible open source license. | ||
* | ||
* Modifications Copyright OpenSearch Contributors. See | ||
* GitHub history for details. | ||
*/ | ||
const { test } = require('tap'); | ||
const { URL } = require('url'); | ||
const { v4: uuidv4 } = require('uuid'); | ||
const AwsV4Signer = require('../../lib/aws/AwsV4Signer'); | ||
const { AwsV4SignerError } = require('../../lib/aws/errors'); | ||
const { Connection } = require('../../index'); | ||
|
||
test('Sign with SigV4', (t) => { | ||
t.plan(2); | ||
|
||
const mockCreds = { | ||
accessKeyId: uuidv4(), | ||
secretAccessKey: uuidv4(), | ||
}; | ||
|
||
const mockRegion = 'us-west-2'; | ||
|
||
const AwsV4SignerOptions = { credentials: mockCreds, region: mockRegion }; | ||
|
||
const auth = AwsV4Signer(AwsV4SignerOptions); | ||
|
||
const connection = new Connection({ | ||
url: new URL('https://localhost:9200'), | ||
}); | ||
|
||
const request = connection.buildRequestObject({ | ||
path: '/hello', | ||
method: 'GET', | ||
headers: { | ||
'X-Custom-Test': true, | ||
}, | ||
}); | ||
const signedRequest = auth.buildSignedRequestObject(request); | ||
t.hasProp(signedRequest.headers, 'X-Amz-Date'); | ||
t.hasProp(signedRequest.headers, 'Authorization'); | ||
}); | ||
|
||
test('Sign with SigV4 failure (with empty region)', (t) => { | ||
t.plan(2); | ||
|
||
const mockCreds = { | ||
accessKeyId: uuidv4(), | ||
secretAccessKey: uuidv4(), | ||
}; | ||
|
||
const AwsV4SignerOptions = { credentials: mockCreds }; | ||
|
||
try { | ||
AwsV4Signer(AwsV4SignerOptions); | ||
t.fail('Should fail'); | ||
} catch (err) { | ||
t.ok(err instanceof AwsV4SignerError); | ||
t.equal(err.message, 'Region cannot be empty'); | ||
} | ||
}); | ||
|
||
test('Sign with SigV4 failure (with empty credentials)', (t) => { | ||
t.plan(2); | ||
|
||
const mockRegion = 'us-west-2'; | ||
|
||
const AwsV4SignerOptions = { region: mockRegion }; | ||
|
||
try { | ||
AwsV4Signer(AwsV4SignerOptions); | ||
t.fail('Should fail'); | ||
} catch (err) { | ||
t.ok(err instanceof AwsV4SignerError); | ||
t.equal(err.message, 'Credentials cannot be empty'); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd want it to match above (must have) and be namespaced under
aws
if possible (not sure that's the right thing to do here).WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds right. I fixed the namespace to allow this kind of import.