refactor(security): default useTLS to true on device creation - #1169
refactor(security): default useTLS to true on device creation#1169DevipriyaS17 wants to merge 6 commits into
Conversation
07d469d to
4b26984
Compare
Codecov Report❌ Patch coverage is 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. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
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
useTLSwas present in the JSON body and default it totruewhen omitted. - Add/adjust unit tests to cover the new defaulting behavior and the explicit
useTLS: falsecase.
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. |
There was a problem hiding this comment.
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 whenuseTLSis omitted. To validate the regression end-to-end, remove theuseTLSproperty from the request body and keep the response assertionjsonData.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}",
There was a problem hiding this comment.
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
useTLSwas 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."),
There was a problem hiding this comment.
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 plainboolininternal/entity/dto/v1/device.go). If you want this default to be visible to generated clients/tooling, consider adding a struct tag default (similar todefault:"0"in dto/v1/alarm.go:10 ordefault:"All"in dto/v1/profile.go:46) so the generated spec captures it asdefault: truerather than only in free-text.
fuego.OptionDescription("Create a new device. If useTLS is omitted, it defaults to true."),
a74b732 to
6ff637c
Compare
|
@DevipriyaS17 Also, please add a Postman test with |
cfdb9b3 to
0ffc984
Compare
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
useTLSvalue.Problem
The
POST /api/v1/devicesendpoint accepted whateveruseTLSvaluearrived in the request body. Since Go zero-initialises bool fields,
omitting
useTLSsilently defaulted tofalse, causing plaintextWSMAN traffic as evidenced by:
Post "http://test:16992/wsman"Root Cause
useTLSis aboolindto.Device. Omitting the field from theJSON body results in
false— no explicit default existed at theAPI boundary.
Behaviour Matrix
useTLSomitted"useTLS": falseexplicit"useTLS": trueexplicitLinked URL
device-management-toolkit/sample-web-ui#3480