Skip to content

refactor(security): default useTLS to true on device creation - #1169

Open
DevipriyaS17 wants to merge 6 commits into
mainfrom
CM-351
Open

refactor(security): default useTLS to true on device creation#1169
DevipriyaS17 wants to merge 6 commits into
mainfrom
CM-351

Conversation

@DevipriyaS17

@DevipriyaS17 DevipriyaS17 commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes WSMAN connections to AMT devices now default to TLS
(port 16993/HTTPS) instead of plaintext (port 16992/HTTP) when a
device is created without an explicit useTLS value.

Problem

The POST /api/v1/devices endpoint accepted whatever useTLS value
arrived in the request body. Since Go zero-initialises bool fields,
omitting useTLS silently defaulted to false, causing plaintext
WSMAN traffic as evidenced by:

  • Console logs: Post "http://test:16992/wsman"
  • tcpdump: full plaintext SOAP XML payload visible on the wire

Root Cause

useTLS is a bool in dto.Device. Omitting the field from the
JSON body results in false — no explicit default existed at the
API boundary.

Behaviour Matrix

Scenario Before After
Create device, useTLS omitted HTTP/16992 HTTPS/16993
Create device, "useTLS": false explicit HTTP/16992 HTTP/16992
Create device, "useTLS": true explicit HTTPS/16993 HTTPS/16993

Linked URL

device-management-toolkit/sample-web-ui#3480

@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 69.44444% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 50.19%. Comparing base (7ccfa79) to head (b6b0f27).

Files with missing lines Patch % Lines
internal/controller/httpapi/v1/devices.go 65.78% 7 Missing and 6 partials ⚠️
internal/controller/httpapi/v1/profiles.go 22.22% 5 Missing and 2 partials ⚠️
internal/controller/openapi/devices.go 0.00% 1 Missing ⚠️
pkg/logger/logger.go 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1169      +/-   ##
==========================================
+ Coverage   50.16%   50.19%   +0.02%     
==========================================
  Files         147      147              
  Lines       13574    13614      +40     
==========================================
+ Hits         6810     6834      +24     
- Misses       6172     6188      +16     
  Partials      592      592              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens device creation security by defaulting WSMAN connections to TLS (HTTPS/16993) when useTLS is omitted from POST /api/v1/devices, while still honoring explicit useTLS: false.

Changes:

  • Update the v1 device insert handler to detect whether useTLS was present in the JSON body and default it to true when omitted.
  • Add/adjust unit tests to cover the new defaulting behavior and the explicit useTLS: false case.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
internal/controller/httpapi/v1/devices.go Switches to body-caching JSON bind, inspects provided JSON keys, and defaults UseTLS to true when omitted.
internal/controller/httpapi/v1/devices_test.go Adds tests for defaulting behavior / explicit false, and updates an existing insert test expectation to include UseTLS: true.

Comment thread internal/controller/httpapi/v1/devices.go Outdated
Comment thread internal/controller/httpapi/v1/devices_test.go Outdated
Comment thread internal/controller/httpapi/v1/devices.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

integration-test/collections/console_mps_apis.postman_collection.json:1637

  • The Postman "Create Device" request explicitly sends "useTLS": true, so it doesn't actually exercise the new defaulting behavior when useTLS is omitted. To validate the regression end-to-end, remove the useTLS property from the request body and keep the response assertion jsonData.useTLS === true.
							"raw": "{\r\n    \"guid\": \"143e4567-e89b-12d3-a456-426614174000\",\r\n    \"friendlyName\": \"friendlyName\",\r\n    \"hostname\": \"hostname\",\r\n    \"tags\": [],\r\n    \"mpsusername\": \"admin\",\r\n    \"useTLS\": true,\r\n    \"deviceInfo\": {\r\n        \"fwVersion\": \"16.1.30\",\r\n        \"fwBuild\": \"3400\",\r\n        \"fwSku\": \"11\",\r\n        \"discovered\": true,\r\n        \"firstDiscovered\": \"2026-05-20T00:00:00Z\",\r\n        \"currentMode\": \"Admin\",\r\n        \"features\": \"SOL,IDER,KVM\",\r\n        \"ipAddress\": \"10.0.0.12\",\r\n        \"lastSynced\": \"2026-05-21T00:00:00Z\",\r\n        \"tlsMode\": \"TLS 1.2\",\r\n        \"upid\": {\r\n            \"oemPlatformIdType\": \"Not Set (0)\",\r\n            \"oemId\": \"\",\r\n            \"csmeId\": \"4A45A39C5ED9462082510000\"\r\n        },\r\n        \"amtEnabledInBIOS\": true,\r\n        \"meInterfaceVersion\": \"16.1.25.2124\",\r\n        \"dhcpEnabled\": true,\r\n        \"certHashes\": [\r\n            \"a1b2c3\",\r\n            \"d4e5f6\"\r\n        ],\r\n        \"lmsInstalled\": true,\r\n        \"lmsVersion\": \"2410.5.0.0\",\r\n        \"osName\": \"linux\",\r\n        \"osVersion\": \"6.8.0-51-generic\",\r\n        \"osDistro\": \"Ubuntu 24.04 LTS\",\r\n        \"cpuModel\": \"Intel(R) Core(TM) Ultra 7 165H\",\r\n        \"osIpAddress\": \"10.49.76.163\",\r\n        \"ethernetAdapterCount\": 2,\r\n        \"monitorConnected\": true,\r\n        \"ieee8021xEnabled\": false\r\n    }\r\n}",

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (2)

internal/controller/httpapi/v1/devices.go:211

  • In the create-device handler, calling providedJSONFields() just to detect whether top-level useTLS was present does extra work (it lowercases and recursively flattens nested objects) on every device create. A lighter check against the top-level JSON keys would avoid unnecessary recursion/cpu while keeping the same defaulting behavior.
	fields, err := providedJSONFields(c)

internal/controller/openapi/devices.go:113

  • The OpenAPI route description changed here; per CLAUDE.md (OpenAPI spec section) the generated spec (doc/openapi.json) should be regenerated and committed whenever spec-reachable routes/descriptions change, otherwise SwaggerHub/integrators may see stale docs.
		fuego.OptionDescription("Create a new device. If useTLS is omitted, it defaults to true."),

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

internal/controller/openapi/devices.go:113

  • The route description mentions the default, but the OpenAPI schema for the request body likely still has no field-level default for useTLS (it’s a plain bool in internal/entity/dto/v1/device.go). If you want this default to be visible to generated clients/tooling, consider adding a struct tag default (similar to default:"0" in dto/v1/alarm.go:10 or default:"All" in dto/v1/profile.go:46) so the generated spec captures it as default: true rather than only in free-text.
		fuego.OptionDescription("Create a new device. If useTLS is omitted, it defaults to true."),

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

sudhir-intc
sudhir-intc previously approved these changes Aug 5, 2026
@sudhir-intc sudhir-intc changed the title fix(security): default useTLS to true on device creation refactor(security): default useTLS to true on device creation Aug 14, 2026
@madhavilosetty-intel

Copy link
Copy Markdown
Contributor

@DevipriyaS17 allowSelfSigned should also default to true, along with useTLS, to keep the behavior consistent with the sample-web-ui PR device-management-toolkit/sample-web-ui#3480 where both options are enabled by default.

Also, please add a Postman test with "useTLS": false and "allowSelfSigned": false and verify both remain false. This will make sure the explicit-false behavior doesn’t regress later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants