From 7e6fc4068ea3dd9059aa9e1084d9808fbf57da99 Mon Sep 17 00:00:00 2001 From: Mike Date: Tue, 22 Nov 2022 10:30:40 -0700 Subject: [PATCH 1/9] ci: update checkout to v3, update semantic pr checks (#737) --- .github/workflows/api-test.yml | 2 +- .github/workflows/changelog.yml | 2 +- .github/workflows/codeql-analysis.yml | 2 +- .github/workflows/docker-image-ci.yml | 2 +- .github/workflows/docker-image.yml | 2 +- .github/workflows/node.js.yml | 2 +- .github/workflows/semantic.yml | 17 ++++++++--------- 7 files changed, 14 insertions(+), 15 deletions(-) diff --git a/.github/workflows/api-test.yml b/.github/workflows/api-test.yml index 611c7ba0c..1175be70a 100644 --- a/.github/workflows/api-test.yml +++ b/.github/workflows/api-test.yml @@ -11,7 +11,7 @@ jobs: build: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v3 - run: docker build -f Dockerfile -t mps:${GITHUB_SHA} . - run: docker-compose up -d - run: sleep 30 diff --git a/.github/workflows/changelog.yml b/.github/workflows/changelog.yml index 539f8e419..13c10d718 100644 --- a/.github/workflows/changelog.yml +++ b/.github/workflows/changelog.yml @@ -13,7 +13,7 @@ jobs: build: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v3 with: fetch-depth: 0 - run: docker run -v $PWD:/workdir quay.io/git-chglog/git-chglog --next-tag $(node --eval="process.stdout.write(require('./package.json').version)") --output CHANGELOG.md diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index 75bea0e4c..1aa5ad7da 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -39,7 +39,7 @@ jobs: steps: - name: Checkout repository - uses: actions/checkout@v2 + uses: actions/checkout@v3 # Initializes the CodeQL tools for scanning. - name: Initialize CodeQL diff --git a/.github/workflows/docker-image-ci.yml b/.github/workflows/docker-image-ci.yml index 4dd395be7..1c8417a0e 100644 --- a/.github/workflows/docker-image-ci.yml +++ b/.github/workflows/docker-image-ci.yml @@ -13,7 +13,7 @@ jobs: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v3 - name: Build the Docker image run: docker build . --file Dockerfile --tag vprodemo.azurecr.io/mps:${{ github.sha }} --tag vprodemo.azurecr.io/mps:latest diff --git a/.github/workflows/docker-image.yml b/.github/workflows/docker-image.yml index f7cecb647..af5aaeb3b 100644 --- a/.github/workflows/docker-image.yml +++ b/.github/workflows/docker-image.yml @@ -23,7 +23,7 @@ jobs: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v3 - name: Build the Docker image run: docker build . --file Dockerfile --tag ${{ github.event.inputs.docker_registry }}/${{ github.event.inputs.docker_tag_name }} diff --git a/.github/workflows/node.js.yml b/.github/workflows/node.js.yml index b3602582c..c2930b5ad 100644 --- a/.github/workflows/node.js.yml +++ b/.github/workflows/node.js.yml @@ -23,7 +23,7 @@ jobs: node-version: [14.x, 16.x, 18.x] steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v3 - name: Use Node.js ${{ matrix.node-version }} uses: actions/setup-node@v1 with: diff --git a/.github/workflows/semantic.yml b/.github/workflows/semantic.yml index 3ed686175..d602c0d93 100644 --- a/.github/workflows/semantic.yml +++ b/.github/workflows/semantic.yml @@ -1,22 +1,21 @@ name: "Semantic Pull Request" on: - pull_request_target: - types: - - opened - - edited - - synchronize + pull_request: + types: ['opened', 'edited', 'reopened', 'synchronize'] jobs: main: name: Validate PR and Commits runs-on: ubuntu-latest steps: - - uses: amannn/action-semantic-pull-request@v4 - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - - uses: actions/checkout@v2 + - uses: actions/checkout@v3 with: fetch-depth: 0 - uses: wagoid/commitlint-github-action@v4 with: configFile: .github/commitlint.config.js + - name: Install Dependencies + run: npm install @commitlint/config-conventional + - uses: JulienKode/pull-request-name-linter-action@v0.5.0 + with: + configuration-path: ./.github/commitlint.config.js From fc56834bc31a4d1394bedc00bbc52bc59e082810 Mon Sep 17 00:00:00 2001 From: Orin Eman Date: Mon, 21 Nov 2022 16:15:04 -0800 Subject: [PATCH 2/9] fix: InstanceID and ElementName shouldn't be treated as numeric if they have a leading zero --- src/amt/HttpHandler.ts | 11 ++++++++++- src/amt/httpHandler.test.ts | 14 ++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/src/amt/HttpHandler.ts b/src/amt/HttpHandler.ts index 60ce3958e..4be5a49c0 100644 --- a/src/amt/HttpHandler.ts +++ b/src/amt/HttpHandler.ts @@ -19,6 +19,15 @@ export class connectionParams { digestChallenge?: DigestChallenge } +function myParseNumbers (value: string, name: string): any { + if (name === 'ElementName' || name === 'InstanceID') { + if (value.length > 1 && value.charAt(0) === '0') { + return value + } + } + return xml2js.processors.parseNumbers(value, name) +} + export class HttpHandler { digestChallenge: any authResolve: any @@ -30,7 +39,7 @@ export class HttpHandler { parser: any constructor () { this.stripPrefix = xml2js.processors.stripPrefix - this.parser = new xml2js.Parser({ ignoreAttrs: true, mergeAttrs: false, explicitArray: false, tagNameProcessors: [this.stripPrefix], valueProcessors: [xml2js.processors.parseNumbers, xml2js.processors.parseBooleans] }) + this.parser = new xml2js.Parser({ ignoreAttrs: true, mergeAttrs: false, explicitArray: false, tagNameProcessors: [this.stripPrefix], valueProcessors: [myParseNumbers, xml2js.processors.parseBooleans] }) } wrapIt (connectionParams: connectionParams, data: string): string { diff --git a/src/amt/httpHandler.test.ts b/src/amt/httpHandler.test.ts index b3d863aff..eadc77c87 100644 --- a/src/amt/httpHandler.test.ts +++ b/src/amt/httpHandler.test.ts @@ -19,6 +19,20 @@ it('should throw an error and return null when it parse invalid xml', async () = const result = httpHandler.parseXML(xml) expect(result).toBe(null) }) +it('should preserve leading zeros in ElementName and InstanceID', async () => { + const xml: string = 'http://schemas.xmlsoap.org/ws/2004/08/addressing/role/anonymous1http://schemas.xmlsoap.org/ws/2004/09/enumeration/PullResponseuuid:00000000-8086-8086-8086-0000000022AFhttp://intel.com/wbem/wscim/1/ips-schema/1/IPS_AlarmClockOccurrencetrue01012022-11-30T18:51:00Z' + const result = httpHandler.parseXML(xml) + const expected = { + DeleteOnCompletion: true, + ElementName: '01', + InstanceID: '01', + StartTime: { + Datetime: '2022-11-30T18:51:00Z' + } + } + expect(result?.Envelope?.Body?.PullResponse?.Items?.IPS_AlarmClockOccurrence).toEqual(expected) +}) + it('should parse authentication response header with 1 comma in value', async () => { const digestChallenge = { realm: 'Digest:56ABC7BE224EF620C69EB88F01071DC8', From 98c6a3f4d832c025e6401aa7ed55016098f90876 Mon Sep 17 00:00:00 2001 From: Orin Date: Tue, 22 Nov 2022 09:40:43 -0800 Subject: [PATCH 3/9] fix: issue #661 - don't try to access a zero length body (#734) Co-authored-by: Mike --- src/utils/parseWSManResponseBody.test.ts | 31 +++++++++++++++++++++++- src/utils/parseWSManResponseBody.ts | 2 ++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/src/utils/parseWSManResponseBody.test.ts b/src/utils/parseWSManResponseBody.test.ts index f1d21f8c7..5b61f1b89 100644 --- a/src/utils/parseWSManResponseBody.test.ts +++ b/src/utils/parseWSManResponseBody.test.ts @@ -48,7 +48,7 @@ describe('Check parseWSManResponseBody', () => { expect(response).toEqual(generalSettings) }) - it('Should fail and return null when a message does not contain \r\n', async () => { + it('Should fail and return an empty response when a message does not contain \r\n', async () => { const xmlResponse: HttpZResponseModel = { protocolVersion: 'HTTP/1.1', statusCode: 200, @@ -78,4 +78,33 @@ describe('Check parseWSManResponseBody', () => { const response = parseBody(xmlResponse) expect(response).toBe('') }) + + it('Should fail and return an empty response when there is no message body', async () => { + // See issue #661; this response is documented there. + const xmlResponse: HttpZResponseModel = { + protocolVersion: 'HTTP/1.1', + statusCode: 400, + statusMessage: 'Bad Request', + headersSize: 143, + bodySize: 0, + headers: [ + { + name: 'Date', + value: 'Mon, 1 Aug 2022 18:54:20 GMT' + }, + { + name: 'Server', + value: 'Intel(R) Active Management Technology 15.0.41.2142' + }, + { + name: 'Content-Length', + value: '0' + } + ], + body: null + } + delete xmlResponse.body // Yes, really! It's not there in this case. + const response = parseBody(xmlResponse) + expect(response).toBe('') + }) }) diff --git a/src/utils/parseWSManResponseBody.ts b/src/utils/parseWSManResponseBody.ts index 5d7e125da..6a4b12673 100644 --- a/src/utils/parseWSManResponseBody.ts +++ b/src/utils/parseWSManResponseBody.ts @@ -7,6 +7,8 @@ import { HttpZResponseModel } from 'http-z' export function parseBody (message: HttpZResponseModel): string { let xmlBody: string = '' + // 'Bad' requests to some devices return no body (issue #661) - prevent exceptions below + if (message.bodySize === 0) return '' // parse the body until its length is greater than 5, because body ends with '0\r\n\r\n' while (message.body.text.length > 5) { const chunkLength = message.body.text.indexOf('\r\n') From fd974f81f7e8df5def826b8bb212dc26d41ff7e8 Mon Sep 17 00:00:00 2001 From: Orin Eman Date: Thu, 17 Nov 2022 20:21:43 -0800 Subject: [PATCH 4/9] fix: issue 668 - ensure CIM_KVMRedirectionSAP is present before using it --- src/routes/amt/getAMTFeatures.ts | 1 + src/routes/amt/setAMTFeatures.ts | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/routes/amt/getAMTFeatures.ts b/src/routes/amt/getAMTFeatures.ts index 29dac75d4..8ebcf76f7 100644 --- a/src/routes/amt/getAMTFeatures.ts +++ b/src/routes/amt/getAMTFeatures.ts @@ -46,6 +46,7 @@ export function processAmtRedirectionResponse (amtRedirection: AMT.Models.Redire } export function processKvmRedirectionResponse (kvmRedirection: CIM.Models.KVMRedirectionSAP): boolean { + if (kvmRedirection == null) return false const kvm = (kvmRedirection.EnabledState === Common.Models.CIM_KVM_REDIRECTION_SAP_ENABLED_STATE.Enabled || kvmRedirection.EnabledState === Common.Models.CIM_KVM_REDIRECTION_SAP_ENABLED_STATE.EnabledButOffline) return kvm diff --git a/src/routes/amt/setAMTFeatures.ts b/src/routes/amt/setAMTFeatures.ts index 85e17b85d..c96f7921f 100644 --- a/src/routes/amt/setAMTFeatures.ts +++ b/src/routes/amt/setAMTFeatures.ts @@ -27,8 +27,9 @@ export async function setAMTFeatures (req: Request, res: Response): Promise Date: Tue, 22 Nov 2022 12:16:14 -0700 Subject: [PATCH 5/9] ci(gh-actions): add scopes (#739) --- .github/commitlint.config.js | 5 +++++ .github/semantic.yml | 14 -------------- CONTRIBUTING.md | 21 ++++++++++++++------- 3 files changed, 19 insertions(+), 21 deletions(-) delete mode 100644 .github/semantic.yml diff --git a/.github/commitlint.config.js b/.github/commitlint.config.js index 8822b4b40..9610b4acd 100644 --- a/.github/commitlint.config.js +++ b/.github/commitlint.config.js @@ -7,5 +7,10 @@ module.exports = { 'never', ['sentence-case', 'start-case', 'pascal-case', 'upper-case'], ], + 'scope-enum': [ + 2, + 'always', + ['db', 'api', 'secrets', 'cira', 'apf', 'health', 'utils', 'redir', 'events', 'docker', 'deps', 'deps-dev', 'gh-actions', 'config'] + ] } } \ No newline at end of file diff --git a/.github/semantic.yml b/.github/semantic.yml deleted file mode 100644 index 52e966e50..000000000 --- a/.github/semantic.yml +++ /dev/null @@ -1,14 +0,0 @@ -allowMergeCommits: true -# Always validate the PR title AND all the commits -titleAndCommits: true -types: - - feat - - fix - - docs - - style - - refactor - - perf - - test - - build - - ci - - revert diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8f0a86834..1f16d55d3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -52,13 +52,20 @@ Must be one of the following: Should be one of the following: Modules: -* **common**: A change or addition to the common module -* **tracker**: A change or addition to the tracker module -* **server**: A change or addition to the server module -* **msgs**: A change or addition to msgs module -* **eval**: Any change to benchmark tools -* **deps**: A change to any dependency or 3rd-party library -* **all**: A change that affects all modules +* **apf**: A change or addition to amt port forwarding functionality +* **api**: A change or addition to REST functionality +* **cira**: A change or addition to client initiated remote access functionality +* **config**: A change or addition to service configuration +* **db**: A change or addition to database calls or functionality +* **deps**: A change or addition to dependencies (primarily used by dependabot) +* **deps-dev**: A change or addition to developer dependencies (primarily used by dependabot) +* **docker**: A change or addition to docker file or composition +* **events**: A change or addition to eventing from the service +* **gh-actions**: A change or addition to GitHub actions +* **health**: A change or addition to health checks +* **redir**: A change or addition to redirection functionality +* **secrets**: A change or addition to secret store calls or functionality +* **utils**: A change or addition to the utility functions * *no scope*: If no scope is provided, it is assumed the PR does not apply to the above scopes ### Body From de565d14b547d3c0400fad86e3a2246ae6ab0d10 Mon Sep 17 00:00:00 2001 From: Orin Eman Date: Thu, 17 Nov 2022 13:52:37 -0800 Subject: [PATCH 6/9] fix: issue #729 and use Buffer rather than string for CIRAChannel's sendBuffer Added test for CIRAChannel writeData() binary path --- src/amt/APFProcessor.test.ts | 26 ++++++------ src/amt/APFProcessor.ts | 57 ++++++++++++++------------- src/amt/CIRAChannel.test.ts | 20 +++++++++- src/amt/CIRAChannel.ts | 36 ++++++----------- src/amt/HttpHandler.ts | 15 ++++++- src/amt/httpHandler.test.ts | 22 ++++++++++- src/utils/redirectInterceptor.test.ts | 2 +- 7 files changed, 110 insertions(+), 68 deletions(-) diff --git a/src/amt/APFProcessor.test.ts b/src/amt/APFProcessor.test.ts index 211f660c3..59c69acd4 100644 --- a/src/amt/APFProcessor.test.ts +++ b/src/amt/APFProcessor.test.ts @@ -3,6 +3,7 @@ * SPDX-License-Identifier: Apache-2.0 **********************************************************************/ +import { Buffer } from 'node:buffer' import Common from '../utils/common' import { logger } from '../logging' import APFProcessor, { APFProtocol } from './APFProcessor' @@ -259,9 +260,7 @@ describe('APFProcessor Tests', () => { it('should return 9 if sending entire pending buffer', () => { const fakeCiraChannel: CIRAChannel = { - sendBuffer: { - length: 1000 - }, + sendBuffer: Buffer.alloc(1000), sendcredits: 1000, socket: { write: jest.fn() @@ -287,7 +286,7 @@ describe('APFProcessor Tests', () => { it('should return 9 if sending partial pending buffer', () => { const fakeCiraChannel: CIRAChannel = { - sendBuffer: 'my fake buffer', + sendBuffer: Buffer.from('my fake buffer'), sendcredits: 5, socket: { write: jest.fn() @@ -461,7 +460,7 @@ describe('APFProcessor Tests', () => { const length = 17 const data = '' fakeCiraChannel.closing = 0 - fakeCiraChannel.sendBuffer = 'fake buffer' + fakeCiraChannel.sendBuffer = Buffer.from('fake buffer') fakeCiraChannel.onStateChange = new EventEmitter() const result = APFProcessor.channelOpenConfirmation(fakeCiraSocket, length, data) expect(result).toEqual(17) @@ -475,7 +474,7 @@ describe('APFProcessor Tests', () => { const data = '' const readIntSpy = jest.spyOn(Common, 'ReadInt').mockReturnValue(1) fakeCiraChannel.closing = 0 - fakeCiraChannel.sendBuffer = 'fake buffer' + fakeCiraChannel.sendBuffer = Buffer.from('fake buffer') fakeCiraChannel.onStateChange = new EventEmitter() const result = APFProcessor.channelOpenConfirmation(fakeCiraSocket, length, data) expect(result).toEqual(17) @@ -1139,13 +1138,16 @@ describe('APFProcessor Tests', () => { }) it('should SendChannelData', () => { - APFProcessor.SendChannelData(fakeCiraSocket, channelid, data) - const dataExpected = + writeSpy = jest.spyOn(fakeCiraSocket, 'write') + APFProcessor.SendChannelData(fakeCiraSocket, channelid, Buffer.from(data)) + const dataExpected = Buffer.from( String.fromCharCode(APFProtocol.CHANNEL_DATA) + - Common.IntToStr(channelid) + - Common.IntToStr(data.length) + - data - expect(writeSpy).toHaveBeenCalledWith(fakeCiraSocket, dataExpected) + Common.IntToStr(channelid) + + Common.IntToStr(data.length) + + data, + 'binary' + ) + expect(writeSpy).toHaveBeenCalledWith(dataExpected) }) it('should SendChannelWindowAdjust', () => { diff --git a/src/amt/APFProcessor.ts b/src/amt/APFProcessor.ts index acca507c5..f635f075e 100644 --- a/src/amt/APFProcessor.ts +++ b/src/amt/APFProcessor.ts @@ -3,9 +3,11 @@ * SPDX-License-Identifier: Apache-2.0 **********************************************************************/ +import { Buffer } from 'node:buffer' import { logger, messages } from '../logging' import Common from '../utils/common' import { CIRASocket } from '../models/models' +import { CIRAChannel } from './CIRAChannel' import { EventEmitter } from 'stream' const KEEPALIVE_INTERVAL = 30 // 30 seconds is typical keepalive interval for AMT CIRA connection @@ -155,19 +157,8 @@ const APFProcessor = { } cirachannel.sendcredits += ByteToAdd logger.silly(`${messages.MPS_WINDOW_ADJUST}, ${RecipientChannel.toString()}, ${ByteToAdd.toString()}, ${cirachannel.sendcredits}`) - if (cirachannel.state === 2 && cirachannel.sendBuffer != null) { - // Compute how much data we can send - if (cirachannel.sendBuffer.length <= cirachannel.sendcredits) { - // Send the entire pending buffer - APFProcessor.SendChannelData(cirachannel.socket, cirachannel.amtchannelid, cirachannel.sendBuffer) - cirachannel.sendcredits -= cirachannel.sendBuffer.length - delete cirachannel.sendBuffer - } else { - // Send a part of the pending buffer - APFProcessor.SendChannelData(cirachannel.socket, cirachannel.amtchannelid, cirachannel.sendBuffer.substring(0, cirachannel.sendcredits)) - cirachannel.sendBuffer = cirachannel.sendBuffer.substring(cirachannel.sendcredits) - cirachannel.sendcredits = 0 - } + if (cirachannel.state === 2) { + APFProcessor.SendPendingData(cirachannel) } return 9 }, @@ -232,19 +223,7 @@ const APFProcessor = { } else { cirachannel.state = 2 // Send any pending data - if (cirachannel.sendBuffer != null) { - if (cirachannel.sendBuffer.length <= cirachannel.sendcredits) { - // Send the entire pending buffer - APFProcessor.SendChannelData(cirachannel.socket, cirachannel.amtchannelid, cirachannel.sendBuffer) - cirachannel.sendcredits -= cirachannel.sendBuffer.length - delete cirachannel.sendBuffer - } else { - // Send a part of the pending buffer - APFProcessor.SendChannelData(cirachannel.socket, cirachannel.amtchannelid, cirachannel.sendBuffer.substring(0, cirachannel.sendcredits)) - cirachannel.sendBuffer = cirachannel.sendBuffer.substring(cirachannel.sendcredits) - cirachannel.sendcredits = 0 - } - } + APFProcessor.SendPendingData(cirachannel) // Indicate the channel is open if (cirachannel.onStateChange) { cirachannel.onStateChange.emit('stateChange', cirachannel.state) @@ -535,8 +514,31 @@ const APFProcessor = { APFProcessor.Write(socket, String.fromCharCode(APFProtocol.CHANNEL_CLOSE) + Common.IntToStr(channelid)) }, - SendChannelData: (socket: CIRASocket, channelid, data): void => { + SendPendingData: (cirachannel: CIRAChannel): void => { + if (cirachannel.sendBuffer != null) { + if (cirachannel.sendBuffer.length <= cirachannel.sendcredits) { + // Send the entire pending buffer + APFProcessor.SendChannelData(cirachannel.socket, cirachannel.amtchannelid, cirachannel.sendBuffer) + cirachannel.sendcredits -= cirachannel.sendBuffer.length + delete cirachannel.sendBuffer + } else { + // Send a part of the pending buffer + APFProcessor.SendChannelData(cirachannel.socket, cirachannel.amtchannelid, cirachannel.sendBuffer.subarray(0, cirachannel.sendcredits)) + cirachannel.sendBuffer = cirachannel.sendBuffer.subarray(cirachannel.sendcredits) + cirachannel.sendcredits = 0 + } + } + }, + + SendChannelData: (socket: CIRASocket, channelid, data: Buffer): void => { logger.silly(`${messages.MPS_SEND_CHANNEL_DATA}, ${channelid}`) + const b = Buffer.allocUnsafe(9 + data.length) + b[0] = APFProtocol.CHANNEL_DATA + b.writeUInt32BE(channelid, 1) + b.writeUInt32BE(data.length, 5) + data.copy(b, 9) + socket.write(b) + /* APFProcessor.Write( socket, String.fromCharCode(APFProtocol.CHANNEL_DATA) + @@ -544,6 +546,7 @@ const APFProcessor = { Common.IntToStr(data.length) + data ) + */ }, SendChannelWindowAdjust: (socket: CIRASocket, channelid, bytestoadd): void => { diff --git a/src/amt/CIRAChannel.test.ts b/src/amt/CIRAChannel.test.ts index 945a3fe53..2afa23b2f 100644 --- a/src/amt/CIRAChannel.test.ts +++ b/src/amt/CIRAChannel.test.ts @@ -83,9 +83,27 @@ describe('CIRA Channel', () => { expect(sendChannelSpy).toHaveBeenCalledTimes(1) expect(ciraChannel.sendcredits).toBe(0) }) + it('should write binary data to channel', async () => { + // Tests both the binary data path and appending to the sendBuffer. + // There are enough send credits for the initial sendBuffer only, + // so the string written should appear in sendBuffer. + ciraChannel.state = 2 + ciraChannel.sendcredits = 10 + ciraChannel.sendBuffer = Buffer.alloc(10) + const data = String.fromCharCode(1, 2, 3, 4, 0xC0, 5) + const sendChannelSpy = jest.spyOn(APFProcessor, 'SendChannelData').mockImplementation(() => {}) + + const writePromise = ciraChannel.writeData(data, null) + const responseData = await writePromise + + expect(responseData).toEqual(null) + expect(sendChannelSpy).toHaveBeenCalledTimes(1) + expect(ciraChannel.sendcredits).toBe(0) + expect(ciraChannel.sendBuffer).toEqual(Buffer.from(data, 'binary')) + }) it('should resolve if data does not contain messageId', async () => { ciraChannel.state = 2 - ciraChannel.sendcredits = 116 + ciraChannel.sendcredits = 97 const data = 'KVMR' const params: connectionParams = { guid: '4c4c4544-004b-4210-8033-b6c04f504633', diff --git a/src/amt/CIRAChannel.ts b/src/amt/CIRAChannel.ts index aeabe4452..f9cae7ef0 100644 --- a/src/amt/CIRAChannel.ts +++ b/src/amt/CIRAChannel.ts @@ -4,6 +4,7 @@ **********************************************************************/ // import httpZ, { HttpZResponseModel } from 'http-z' +import { Buffer } from 'node:buffer' import { CIRASocket } from '../models/models' import APFProcessor from './APFProcessor' import { connectionParams, HttpHandler } from './HttpHandler' @@ -21,7 +22,7 @@ export class CIRAChannel { amtCiraWindow: number ciraWindow: number write?: (data: string) => Promise - sendBuffer?: string = null + sendBuffer?: Buffer = null amtchannelid?: number closing?: number onStateChange?: EventEmitter // (state: number) => void @@ -83,34 +84,21 @@ export class CIRAChannel { } else { this.resolve = resolve } - let wsmanRequest: any = data + if (this.state === 0) return reject(new Error('Closed'))// return false + let wsmanRequest: Buffer if (params != null) { // this is an API Call wsmanRequest = this.httpHandler.wrapIt(params, data) + } else { + wsmanRequest = Buffer.from(data, 'binary') } - if (this.state === 0) return reject(new Error('Closed'))// return false - if (this.state === 1 || this.sendcredits === 0 || this.sendBuffer != null) { - if (this.sendBuffer == null) { - this.sendBuffer = wsmanRequest - } else { - this.sendBuffer += wsmanRequest - } - if (messageId == null) { - return resolve(null) - } else { return } + if (this.sendBuffer == null) { + this.sendBuffer = wsmanRequest + } else { + this.sendBuffer = Buffer.concat([this.sendBuffer, wsmanRequest]) } - // Compute how much data we can send - if (wsmanRequest?.length <= this.sendcredits) { - // Send the entire message - APFProcessor.SendChannelData(this.socket, this.amtchannelid, wsmanRequest) - this.sendcredits -= wsmanRequest.length - if (messageId == null) { - return resolve(null) - } else { return } + if (this.state !== 1 && this.sendcredits !== 0) { + APFProcessor.SendPendingData(this) } - // Send a part of the message - this.sendBuffer = wsmanRequest.substring(this.sendcredits) - APFProcessor.SendChannelData(this.socket, this.amtchannelid, wsmanRequest.substring(0, this.sendcredits)) - this.sendcredits = 0 if (messageId == null) { resolve(null) } diff --git a/src/amt/HttpHandler.ts b/src/amt/HttpHandler.ts index 4be5a49c0..78b81d336 100644 --- a/src/amt/HttpHandler.ts +++ b/src/amt/HttpHandler.ts @@ -42,7 +42,7 @@ export class HttpHandler { this.parser = new xml2js.Parser({ ignoreAttrs: true, mergeAttrs: false, explicitArray: false, tagNameProcessors: [this.stripPrefix], valueProcessors: [myParseNumbers, xml2js.processors.parseBooleans] }) } - wrapIt (connectionParams: connectionParams, data: string): string { + wrapIt (connectionParams: connectionParams, data: string): Buffer { try { const url = '/wsman' const action = 'POST' @@ -71,6 +71,7 @@ export class HttpHandler { }) message += `Authorization: ${authorizationRequestHeader}\r\n` } + /* // Use Chunked-Encoding message += Buffer.from([ `Host: ${connectionParams.guid}:${connectionParams.port}`, @@ -82,6 +83,15 @@ export class HttpHandler { '\r\n' ].join('\r\n'), 'utf8') return message + */ + const dataBuffer = Buffer.from(data, 'utf8') + message += `Host: ${connectionParams.guid}:${connectionParams.port}\r\nContent-Length: ${dataBuffer.length}\r\n\r\n` + const buffer = Buffer.concat([Buffer.from(message, 'utf8'), dataBuffer]) + if (dataBuffer.length !== data.length) { + logger.silly(`wrapIt data length mismatch: Buffer.length = ${dataBuffer.length}, string.length = ${data.length}`) + logger.silly(buffer.toString('utf8')) + } + return buffer } catch (err) { logger.error(`${messages.CREATE_HASH_STRING_FAILED}:`, err.message) return null @@ -117,7 +127,8 @@ export class HttpHandler { parseXML (xmlBody: string): any { let wsmanResponse: string - this.parser.parseString(xmlBody, (err, result) => { + const xmlDecoded: string = Buffer.from(xmlBody, 'binary').toString('utf8') + this.parser.parseString(xmlDecoded, (err, result) => { if (err) { logger.error(`${messages.XML_PARSE_FAILED}:`, err) wsmanResponse = null diff --git a/src/amt/httpHandler.test.ts b/src/amt/httpHandler.test.ts index eadc77c87..db53e1d5b 100644 --- a/src/amt/httpHandler.test.ts +++ b/src/amt/httpHandler.test.ts @@ -109,7 +109,27 @@ it('should return a WSMan request', async () => { password: 'P@ssw0rd' } const result = httpHandler.wrapIt(params, xmlRequestBody) - expect(result).toContain('Authorization') + expect(result.toString()).toContain('Authorization') +}) +it('should properly encode UTF8 characters', async () => { + // À is codepoint 0x00C0, [0xC3, 0x80] when UTF8 encoded... + const xmlRequestBody = 'FooÀbar' + const digestChallenge = { + realm: 'Digest:56ABC7BE224EF620C69EB88F01071DC8', + nonce: 'fVNueyEAAAAAAAAAcO8WqJ8s+WdyFUIY', + stale: 'false', + qop: 'auth' + } + const params: connectionParams = { + guid: '4c4c4544-004b-4210-8033-b6c04f504633', + port: 16992, + digestChallenge: digestChallenge, + username: 'admin', + password: 'P@ssw0rd' + } + const result: Buffer = httpHandler.wrapIt(params, xmlRequestBody) + expect(result.toString('binary')).toContain('Foo' + String.fromCharCode(0xC3, 0x80) + 'bar') + expect(result.toString('binary')).toContain('\r\nContent-Length: 19\r\n') }) it('should return a null when no xml is passed to wrap a WSMan request', async () => { const digestChallenge = { diff --git a/src/utils/redirectInterceptor.test.ts b/src/utils/redirectInterceptor.test.ts index e68006503..53974bc10 100644 --- a/src/utils/redirectInterceptor.test.ts +++ b/src/utils/redirectInterceptor.test.ts @@ -796,7 +796,7 @@ test('handleAuthenticateSession DIGEST with user, pass and digestRealm', () => { r += String.fromCharCode(ws.authCNonce.length) // CNonce Length r += ws.authCNonce // CNonce r += String.fromCharCode(ncs.length) // NonceCount Length - r += ncs // NonceCount // NonceCount + r += ncs // NonceCount r += String.fromCharCode(digest.length) // Response Length r += digest // Response r += String.fromCharCode(amt.digestQOP.length) // QOP Length From c29d80dfc074e8d3d6396a9be3f9de7a105caa6b Mon Sep 17 00:00:00 2001 From: Orin Eman Date: Sun, 27 Nov 2022 14:02:03 -0800 Subject: [PATCH 7/9] fix: issue #743 - close channel on Request 'aborted' event Ensure channel is closed if an API request is aborted. Improved afterResponse test to ensure CloseChannel is called. --- src/server/webserver.test.ts | 34 +++++++++++++++++++++++++++++++--- src/server/webserver.ts | 7 +++++++ 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/src/server/webserver.test.ts b/src/server/webserver.test.ts index fe402341c..367fd4c9e 100644 --- a/src/server/webserver.test.ts +++ b/src/server/webserver.test.ts @@ -210,7 +210,8 @@ describe('webserver tests', () => { ciraHandler: { channel: 2 } - } + }, + on: jest.fn() } const res: Express.Response = { on: jest.fn() @@ -231,14 +232,22 @@ describe('webserver tests', () => { CloseChannel: jest.fn() } } - } + }, + on: jest.fn(), + removeListener: jest.fn() } const res: Express.Response = { removeListener: jest.fn() } const afterResponseSpy = jest.spyOn(web, 'afterResponse') + const closeChannelSpy = jest.spyOn((req as any).deviceAction.ciraHandler.channel, 'CloseChannel') + const reqRemoveListenerSpy = jest.spyOn(req as any, 'removeListener') + const resRemoveListenerSpy = jest.spyOn(res as any, 'removeListener') web.afterResponse(req as any, res as any) expect(afterResponseSpy).toHaveBeenCalledTimes(1) + expect(closeChannelSpy).toHaveBeenCalledTimes(1) + expect(reqRemoveListenerSpy).toHaveBeenCalledTimes(1) + expect(resRemoveListenerSpy).toHaveBeenCalledTimes(2) }) it('test afterResponse with undefined channel', () => { const req: Express.Request = { @@ -246,7 +255,9 @@ describe('webserver tests', () => { ciraHandler: { channel: null } - } + }, + on: jest.fn(), + removeListener: jest.fn() } const res: Express.Response = { removeListener: jest.fn() @@ -255,6 +266,23 @@ describe('webserver tests', () => { web.afterResponse(req as any, res as any) expect(afterResponseSpy).toHaveBeenCalledTimes(2) }) + it('test onAborted calls afterResponse', () => { + const req: Express.Request = { + deviceAction: { + ciraHandler: { + channel: null + } + }, + on: jest.fn(), + removeListener: jest.fn() + } + const res: Express.Response = { + removeListener: jest.fn() + } + const afterResponseSpy = jest.spyOn(web, 'afterResponse') + web.onAborted(req as any, res as any) + expect(afterResponseSpy).toHaveBeenCalledTimes(3) + }) }) describe('relayconnection', () => { diff --git a/src/server/webserver.ts b/src/server/webserver.ts index ae797eb40..4456ab876 100644 --- a/src/server/webserver.ts +++ b/src/server/webserver.ts @@ -81,9 +81,15 @@ export class WebServer { appUseCall (req: Request, res: Response, next: NextFunction): void { res.on('finish', this.afterResponse.bind(this, req, res)) res.on('close', this.afterResponse.bind(this, req, res)) + req.on('aborted', this.onAborted.bind(this, req, res)) next() } + onAborted (req: Request, res: Response): void { + logger.debug(`Request aborted: ${req.url ?? 'undefined'}`) + this.afterResponse(req, res) + } + afterResponse (req: Request, res: Response): void { if (req.deviceAction?.ciraHandler?.channel) { logger.debug(messages.EOR_CLOSING_CHANNEL) @@ -91,6 +97,7 @@ export class WebServer { } res.removeListener('finish', this.afterResponse) res.removeListener('close', this.afterResponse) + req.removeListener('aborted', this.onAborted) // actions after response } From 8a5d5a3c535255177a95dab7be301622989f3a3f Mon Sep 17 00:00:00 2001 From: Mike Date: Mon, 28 Nov 2022 11:03:30 -0700 Subject: [PATCH 8/9] feat(health): add waits for db and vault to be available to respond to request closes: AB#14652 closes MPS should wait for vault to be unsealed - CIRA connections fail #614 --- package-lock.json | 11 +++++++++++ package.json | 3 ++- src/index.test.ts | 31 +++++++++++++++++++++++++++++++ src/index.ts | 24 ++++++++++++++++++++++++ 4 files changed, 68 insertions(+), 1 deletion(-) diff --git a/package-lock.json b/package-lock.json index 73496d9c7..bfd3041fd 100644 --- a/package-lock.json +++ b/package-lock.json @@ -14,6 +14,7 @@ "bottleneck": "^2.19.5", "consul": "^1.1.0", "cors": "^2.8.5", + "exponential-backoff": "^3.1.0", "express": "^4.18.2", "express-validator": "^6.14.2", "got": "^11.8.5", @@ -3636,6 +3637,11 @@ "node": "^10.13.0 || ^12.13.0 || ^14.15.0 || >=15.0.0" } }, + "node_modules/exponential-backoff": { + "version": "3.1.0", + "resolved": "https://registry.npmjs.org/exponential-backoff/-/exponential-backoff-3.1.0.tgz", + "integrity": "sha512-oBuz5SYz5zzyuHINoe9ooePwSu0xApKWgeNzok4hZ5YKXFh9zrQBEM15CXqoZkJJPuI2ArvqjPQd8UKJA753XA==" + }, "node_modules/express": { "version": "4.18.2", "resolved": "https://registry.npmjs.org/express/-/express-4.18.2.tgz", @@ -10860,6 +10866,11 @@ "jest-message-util": "^27.5.1" } }, + "exponential-backoff": { + "version": "3.1.0", + "resolved": "https://registry.npmjs.org/exponential-backoff/-/exponential-backoff-3.1.0.tgz", + "integrity": "sha512-oBuz5SYz5zzyuHINoe9ooePwSu0xApKWgeNzok4hZ5YKXFh9zrQBEM15CXqoZkJJPuI2ArvqjPQd8UKJA753XA==" + }, "express": { "version": "4.18.2", "resolved": "https://registry.npmjs.org/express/-/express-4.18.2.tgz", diff --git a/package.json b/package.json index 9f0a466a6..6fdba9a87 100644 --- a/package.json +++ b/package.json @@ -38,6 +38,7 @@ "bottleneck": "^2.19.5", "consul": "^1.1.0", "cors": "^2.8.5", + "exponential-backoff": "^3.1.0", "express": "^4.18.2", "express-validator": "^6.14.2", "got": "^11.8.5", @@ -76,4 +77,4 @@ "ts-node": "^10.9.1", "typescript": "^4.8.4" } -} \ No newline at end of file +} diff --git a/src/index.test.ts b/src/index.test.ts index 0d3ac3d8b..2c4d3d613 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -4,9 +4,12 @@ **********************************************************************/ import * as indexFile from './index' +import * as exponentialBackoff from 'exponential-backoff' import { logger } from './logging' import VaultSecretManagerService from './secrets/vault' import { Environment } from './utils/Environment' +import { IDB } from './interfaces/IDb' +import { ISecretManagerService } from './interfaces/ISecretManagerService' describe('Index', () => { describe('loadConfig', () => { @@ -170,4 +173,32 @@ describe('Index', () => { expect(result.mps_tls_config.requestCert).toEqual(true) }) }) + + it('should wait for db', async () => { + const backOffSpy = jest.spyOn(exponentialBackoff, 'backOff') + let shouldBeOk = false + const dbMock: IDB = { + query: jest.fn(() => { + if (shouldBeOk) return null + shouldBeOk = true + throw new Error('error') + }) + } as any + await indexFile.waitForDB(dbMock) + expect(backOffSpy).toHaveBeenCalled() + }) + + it('should wait for secret provider', async () => { + const backOffSpy = jest.spyOn(exponentialBackoff, 'backOff') + let shouldBeOk = false + const secretMock: ISecretManagerService = { + health: jest.fn(() => { + if (shouldBeOk) return null + shouldBeOk = true + throw new Error('error') + }) + } as any + await indexFile.waitForSecretProvider(secretMock) + expect(backOffSpy).toHaveBeenCalled() + }) }) diff --git a/src/index.ts b/src/index.ts index 00341c9cc..764d57337 100644 --- a/src/index.ts +++ b/src/index.ts @@ -17,6 +17,7 @@ import { SecretManagerCreatorFactory } from './factories/SecretManagerCreatorFac import { IDB } from './interfaces/IDb' import { WebServer } from './server/webserver' import { MPSServer } from './server/mpsserver' +import { backOff } from 'exponential-backoff' export async function main (): Promise { try { @@ -33,6 +34,8 @@ export async function main (): Promise { // DB initialization const newDB = new DbCreatorFactory() const db = await newDB.getDb() + // wait for db to be ready + await waitForDB(db) await setupSignalHandling(db) @@ -40,6 +43,9 @@ export async function main (): Promise { const newSecrets = new SecretManagerCreatorFactory() const secrets = await newSecrets.getSecretManager(logger) + // wait for secret provider to be ready + await waitForSecretProvider(secrets) + const certs = await loadCertificates(secrets) // MQTT Connection - Creates a static connection to be access across MPS const mqtt: MqttProvider = new MqttProvider() @@ -56,6 +62,24 @@ export async function main (): Promise { } } +export async function waitForDB (db: IDB): Promise { + return await backOff(async () => await db.query('SELECT 1'), { + retry: (e: any, attemptNumber: number) => { + logger.info(`waiting for db[${attemptNumber}] ${e.code || e.message || e}`) + return true + } + }) +} + +export async function waitForSecretProvider (secrets: ISecretManagerService): Promise { + return await backOff(async () => await secrets.health(), { + retry: (e: any, attemptNumber: number) => { + logger.info(`waiting for secret provider[${attemptNumber}] ${e.code || e.message || e}`) + return true + } + }) +} + export function loadConfig (config: any): configType { // To merge ENV variables. consider after lower-casing ENV since our config keys are lowercase if (config.web_auth_enabled) { From 249317a83435f95d83ff443628d181129d8be308 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 28 Nov 2022 20:20:42 +0000 Subject: [PATCH 9/9] build(deps-dev): bump @types/node-forge from 1.3.0 to 1.3.1 Bumps [@types/node-forge](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/node-forge) from 1.3.0 to 1.3.1. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/node-forge) --- updated-dependencies: - dependency-name: "@types/node-forge" dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] --- package-lock.json | 14 +++++++------- package.json | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/package-lock.json b/package-lock.json index bfd3041fd..4e91fd13e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -33,7 +33,7 @@ "@types/express": "^4.17.14", "@types/jest": "^27.4.1", "@types/node": "^16.11.15", - "@types/node-forge": "^1.3.0", + "@types/node-forge": "^1.3.1", "@types/pg": "^8.6.5", "@types/ws": "^8.5.3", "@typescript-eslint/eslint-plugin": "^4.33.0", @@ -1442,9 +1442,9 @@ "integrity": "sha512-fpP+jk2zJ4VW66+wAMFoBJlx1bxmBKx4DUFf68UHgdGCOuyUTDlLWqsaNPJh7xhNDykyJ9eIzAygilP/4WoN8g==" }, "node_modules/@types/node-forge": { - "version": "1.3.0", - "resolved": "https://registry.npmjs.org/@types/node-forge/-/node-forge-1.3.0.tgz", - "integrity": "sha512-yUsIEHG3d81E2c+akGjZAMdVcjbfqMzpMjvpebnTO7pEGfqxCtBzpRV52kR1RETtwJ9fHkLdEjtaM+uMKWpwFA==", + "version": "1.3.1", + "resolved": "https://registry.npmjs.org/@types/node-forge/-/node-forge-1.3.1.tgz", + "integrity": "sha512-hvQ7Wav8I0j9amPXJtGqI/Yx70zeF62UKlAYq8JPm0nHzjKKzZvo9iR3YI2MiOghZRlOI+tQ2f6D+G6vVf4V2Q==", "dev": true, "dependencies": { "@types/node": "*" @@ -9228,9 +9228,9 @@ "integrity": "sha512-fpP+jk2zJ4VW66+wAMFoBJlx1bxmBKx4DUFf68UHgdGCOuyUTDlLWqsaNPJh7xhNDykyJ9eIzAygilP/4WoN8g==" }, "@types/node-forge": { - "version": "1.3.0", - "resolved": "https://registry.npmjs.org/@types/node-forge/-/node-forge-1.3.0.tgz", - "integrity": "sha512-yUsIEHG3d81E2c+akGjZAMdVcjbfqMzpMjvpebnTO7pEGfqxCtBzpRV52kR1RETtwJ9fHkLdEjtaM+uMKWpwFA==", + "version": "1.3.1", + "resolved": "https://registry.npmjs.org/@types/node-forge/-/node-forge-1.3.1.tgz", + "integrity": "sha512-hvQ7Wav8I0j9amPXJtGqI/Yx70zeF62UKlAYq8JPm0nHzjKKzZvo9iR3YI2MiOghZRlOI+tQ2f6D+G6vVf4V2Q==", "dev": true, "requires": { "@types/node": "*" diff --git a/package.json b/package.json index 6fdba9a87..c3ae5ee68 100644 --- a/package.json +++ b/package.json @@ -57,7 +57,7 @@ "@types/express": "^4.17.14", "@types/jest": "^27.4.1", "@types/node": "^16.11.15", - "@types/node-forge": "^1.3.0", + "@types/node-forge": "^1.3.1", "@types/pg": "^8.6.5", "@types/ws": "^8.5.3", "@typescript-eslint/eslint-plugin": "^4.33.0",