Skip to content

fix: [backport] support mTLS in 1.0 Binary and Structured emitters #106

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

Merged
merged 1 commit into from
May 5, 2020
Merged

fix: [backport] support mTLS in 1.0 Binary and Structured emitters #106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
46 changes: 18 additions & 28 deletions lib/bindings/http/emitter_binary.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,58 +2,48 @@ var axios = require("axios");
var empty = require("is-empty");

const Constants = require("./constants.js");
const defaults = {};
defaults[Constants.HEADERS] = {};
defaults[Constants.HEADERS][Constants.HEADER_CONTENT_TYPE] = Constants.DEFAULT_CONTENT_TYPE;

function BinaryHTTPEmitter(config, headerByGetter, extensionPrefix){
this.config = JSON.parse(JSON.stringify(config));
this.config = Object.assign({}, defaults, config);
this.headerByGetter = headerByGetter;
this.extensionPrefix = extensionPrefix;

this.config[Constants.HEADERS] =
(!this.config[Constants.HEADERS]
? {}
: this.config[Constants.HEADERS]);

// default is json
if(!this.config[Constants.HEADERS][Constants.HEADER_CONTENT_TYPE]){
this.config[Constants.HEADERS][Constants.HEADER_CONTENT_TYPE] =
Constants.DEFAULT_CONTENT_TYPE;
}
}

BinaryHTTPEmitter.prototype.emit = function(cloudevent) {
// Create new request object
var _config = JSON.parse(JSON.stringify(this.config));

// Always set stuff in _config
var _headers = _config[Constants.HEADERS];
BinaryHTTPEmitter.prototype.emit = function (cloudevent) {
const config = Object.assign({}, this.config);
const headers = Object.assign({}, this.config[Constants.HEADERS]);

Object.keys(this.headerByGetter)
.filter((getter) => cloudevent[getter]())
.forEach((getter) => {
let header = this.headerByGetter[getter];
_headers[header.name] =
const header = this.headerByGetter[getter];
headers[header.name] =
header.parser(
cloudevent[getter]()
);
});

// Set the cloudevent payload
let formatted = cloudevent.format();
const formatted = cloudevent.format();
let data = formatted.data;
data = (formatted.data_base64 ? formatted.data_base64: data);

_config[Constants.DATA_ATTRIBUTE] = data;

// Have extensions?
var exts = cloudevent.getExtensions();
const exts = cloudevent.getExtensions();
Object.keys(exts)
.filter((ext) => Object.hasOwnProperty.call(exts, ext))
.forEach((ext) => {
_headers[this.extensionPrefix + ext] = exts[ext];
headers[this.extensionPrefix + ext] = exts[ext];
});

// Return the Promise
return axios.request(_config);
};
config[Constants.DATA_ATTRIBUTE] = data;
config.headers = headers;

// Return the Promise
Copy link
Member

Choose a reason for hiding this comment

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

Formatting.

return axios.request(config);
};

module.exports = BinaryHTTPEmitter;
27 changes: 10 additions & 17 deletions lib/bindings/http/emitter_structured.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,30 +1,23 @@
var axios = require("axios");

const Constants = require("./constants.js");
const defaults = {};
Copy link
Member

Choose a reason for hiding this comment

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

Why are we creating an empty object and then adding properties to it?

const defaults = {
  [Constants.HEADERS]: {
    [Constants.HEADER_CONTENT_TYPE]: Constants.DEFAULT_CE_CONTENT_TYPE
  }
};

Copy link
Member Author

Choose a reason for hiding this comment

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

You introduced new syntax to me. I wasn't aware that you could bracket a nested property as a field name.

Copy link
Member

Choose a reason for hiding this comment

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

OK. This is a feature of JavaScript ES2015. Here's a reference:
https://stackoverflow.com/a/19837961

Copy link
Contributor

Choose a reason for hiding this comment

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

I did not know either! Looks good

defaults[Constants.HEADERS] = {};
defaults[Constants.HEADERS][Constants.HEADER_CONTENT_TYPE] = Constants.DEFAULT_CE_CONTENT_TYPE;

function StructuredHTTPEmitter(configuration){
this.config = JSON.parse(JSON.stringify(configuration));

this.config[Constants.HEADERS] =
(!this.config[Constants.HEADERS]
? {}
: this.config[Constants.HEADERS]);

if(!this.config[Constants.HEADERS][Constants.HEADER_CONTENT_TYPE]){
this.config[Constants.HEADERS][Constants.HEADER_CONTENT_TYPE] =
Constants.DEFAULT_CE_CONTENT_TYPE;
}
this.config = Object.assign({}, defaults, configuration);
Copy link
Member

Choose a reason for hiding this comment

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

Why are we using Object.assign when we can just use the native JavaScript object spread operator?

this.config = {...defaults, ...configuration};

Copy link
Member Author

Choose a reason for hiding this comment

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

The commit happened before we dropped support for early versions of Node.js so this would not have passed CI. It's a reasonable change for master now that we've dropped 6 and 8.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure of the CI system yet but Node versions < 10 were end of life Dec 31, 2019. Completely understandable, but we should only use non-deprecated versions of Node in our CI.

Copy link
Member Author

Choose a reason for hiding this comment

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

@grant yes, I am aware. That's why I updated CI last month to only use 10 & 12. aa2cef6#diff-354f30a63fb0907d4ad57269548329e3

Copy link
Contributor

Choose a reason for hiding this comment

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

@lance I've created this #119 but I don't think a new PR is necessary maybe if you and the others agree with that practice you can update your PR instead wdyt ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@helio-frota I think submitting a separate PR for that is fine.

}

StructuredHTTPEmitter.prototype.emit = function(cloudevent) {
// Create new request object
var _config = JSON.parse(JSON.stringify(this.config));

StructuredHTTPEmitter.prototype.emit = function (cloudevent) {
// Set the cloudevent payload
_config[Constants.DATA_ATTRIBUTE] = cloudevent.format();
this.config[Constants.DATA_ATTRIBUTE] = cloudevent.format();

// Return the Promise
return axios.request(_config);
return axios.request(this.config).then(response => {
delete this.config[Constants.DATA_ATTRIBUTE];
return response;
});
Comment on lines +17 to +20
Copy link
Member

Choose a reason for hiding this comment

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

This is very confusing.

Why don't we just do this?

const res = await axios.request(this.config);
delete this.config[Constants.DATA_ATTRIBUTE];
return res;

The outer function needs to be async of course.

Copy link
Member Author

@lance lance Apr 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The change you suggest introduces a race condition.

Copy link
Member

Choose a reason for hiding this comment

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

No, where's the race? That's not how Node executes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I vote for then notation.

Copy link
Member Author

Choose a reason for hiding this comment

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

@grant try this in your browser https://gist.github.com/lance/ff1bf6d2da191ad164257349486c4c95

Copy link
Member Author

Choose a reason for hiding this comment

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

Although, playing around with this code reveals to me that the race condition exists even using then(). Boo.

};

module.exports = StructuredHTTPEmitter;
54 changes: 42 additions & 12 deletions test/http_binding_1.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
const expect = require("chai").expect;
const nock = require("nock");
const http = require("http");
const request = require("request");
const https = require("https");
const {asBase64} = require("../lib/utils/fun.js");

const BinaryHTTPEmitter =
require("../lib/bindings/http/emitter_binary_1.js");
const Cloudevent = require("../lib/cloudevent.js");

const v1 = require("../v1/index.js");
const {
Spec,
BinaryHTTPEmitter,
StructuredHTTPEmitter,
Cloudevent
} = require("../v1/index.js");

const type = "com.github.pull.create";
const source = "urn:event:from:myapi/resourse/123";
Expand All @@ -28,7 +28,7 @@ const ext2Name = "extension2";
const ext2Value = "acme";

const cloudevent =
new Cloudevent(v1.Spec)
new Cloudevent(Spec)
.type(type)
.source(source)
.dataContentType(ceContentType)
Expand All @@ -48,7 +48,7 @@ const httpcfg = {
};

const binary = new BinaryHTTPEmitter(httpcfg);
const structured = new v1.StructuredHTTPEmitter(httpcfg);
const structured = new StructuredHTTPEmitter(httpcfg);

describe("HTTP Transport Binding - Version 1.0", () => {
beforeEach(() => {
Expand All @@ -59,6 +59,21 @@ describe("HTTP Transport Binding - Version 1.0", () => {
});

describe("Structured", () => {
it('works with mTLS authentication', () => {
const event = new StructuredHTTPEmitter({
method: 'POST',
url: `${webhook}/json`,
httpsAgent: new https.Agent({
cert: 'some value',
key: 'other value'
})
})
return event.emit(cloudevent).then(response => {
expect(response.config.headers['Content-Type'])
.to.equal(contentType);
});
});

describe("JSON Format", () => {
it("requires '" + contentType + "' Content-Type in the header", () => {
return structured.emit(cloudevent)
Expand All @@ -81,7 +96,7 @@ describe("HTTP Transport Binding - Version 1.0", () => {
let bindata = Uint32Array.from(dataString, (c) => c.codePointAt(0));
let expected = asBase64(bindata);
let binevent =
new Cloudevent(v1.Spec)
new Cloudevent(Spec)
.type(type)
.source(source)
.dataContentType("text/plain")
Expand All @@ -98,7 +113,7 @@ describe("HTTP Transport Binding - Version 1.0", () => {

it("the payload must have 'data_base64' when data is binary", () => {
let binevent =
new Cloudevent(v1.Spec)
new Cloudevent(Spec)
.type(type)
.source(source)
.dataContentType("text/plain")
Expand All @@ -117,6 +132,21 @@ describe("HTTP Transport Binding - Version 1.0", () => {
});

describe("Binary", () => {
it('works with mTLS authentication', () => {
const event = new BinaryHTTPEmitter({
method: 'POST',
url: `${webhook}/json`,
httpsAgent: new https.Agent({
cert: 'some value',
key: 'other value'
})
})
return event.emit(cloudevent).then(response => {
Copy link
Member

Choose a reason for hiding this comment

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

This can also be converted to ES6.

Copy link
Member Author

Choose a reason for hiding this comment

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

Again - this was prior to dropping Node 6 and 8 support.

expect(response.config.headers['Content-Type'])
.to.equal(cloudevent.getDataContentType());
});
});

describe("JSON Format", () => {
it("requires '" + cloudevent.getDataContentType() + "' Content-Type in the header", () => {
return binary.emit(cloudevent)
Expand All @@ -138,7 +168,7 @@ describe("HTTP Transport Binding - Version 1.0", () => {
let bindata = Uint32Array.from(dataString, (c) => c.codePointAt(0));
let expected = asBase64(bindata);
let binevent =
new Cloudevent(v1.Spec)
new Cloudevent(Spec)
.type(type)
.source(source)
.dataContentType("text/plain")
Expand Down
2 changes: 2 additions & 0 deletions v1/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,7 @@ module.exports = {
BinaryHTTPEmitter,
StructuredHTTPReceiver,
BinaryHTTPReceiver,
Cloudevent: event,
Copy link
Contributor

Choose a reason for hiding this comment

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

Good!

CloudEvent: event,
event
};