Merge 1.4.1 to main#16
Conversation
This release enables specifying the VDOM during configuration. NOTE: previously, this defaulted to "root" but no longer does, you must specify the VDOM. --------- Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io> Co-authored-by: Lee Fine <lfine@keyfactor.com> Co-authored-by: spbsoluble <1661003+spbsoluble@users.noreply.github.com>
* Update integration-manifest.json * Update generated docs --------- Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io>
There was a problem hiding this comment.
Pull request overview
Merges release-1.4 changes introducing VDOM-aware behavior for the Fortigate orchestrator integration and updating store type definitions/scripts and documentation accordingly.
Changes:
- Add VDOM support by passing Store Path into the FortigateStore and validating VDOM scope/availability.
- Update integration/store-type metadata and add helper scripts (kfutil / curl / PowerShell) for creating the Fortigate store type.
- Update README/docsource/CHANGELOG to reflect VDOM requirements and new/unsupported behaviors.
Reviewed changes
Copilot reviewed 12 out of 15 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| Fortigate/FortigateStore.cs | Adds VDOM handling, VDOM validation, and VDOM scoping checks. |
| Fortigate/Management.cs | Passes Store Path (VDOM) into FortigateStore and validates scope. |
| Fortigate/Inventory.cs | Passes Store Path (VDOM) into FortigateStore; handles partial inventory retrieval failures as warnings. |
| Fortigate/Api/cmdb_certificate_resource.cs | Extends request model with vdom field. |
| integration-manifest.json | Updates store type metadata and StorePathDescription to VDOM semantics. |
| scripts/store_types/** | Adds generated scripts to create the Fortigate store type via REST/curl/kfutil. |
| README.md | Updates documentation to describe VDOM behavior and Store Path meaning; updates compatibility matrix. |
| docsource/content.md | Mirrors README overview updates for doc generation. |
| docsource/images/*.png | Adds updated UI screenshots. |
| CHANGELOG.md | Adds a 1.4.0 entry describing the breaking change (Store Path required for VDOM). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| using Keyfactor.Logging; | ||
| using Microsoft.Extensions.Logging; | ||
| using Keyfactor.Orchestrators.Extensions.Interfaces; | ||
| using Org.BouncyCastle.Tls; |
There was a problem hiding this comment.
The using Org.BouncyCastle.Tls; directive appears to be unused in this file, which will generate warnings (and can fail builds if warnings are treated as errors). Remove the unused import (or add the missing code that requires it).
| using Org.BouncyCastle.Tls; |
| try | ||
| { | ||
| PostAsJson(import_certificate_api, cert_resource, parameters); | ||
| PostAsJson(import_certificate_api, cert_resource); |
There was a problem hiding this comment.
Insert() no longer passes the VDOM as a query parameter when calling the FortiGate import endpoint. Previously this call included vdom=root via query params; now PostAsJson(import_certificate_api, cert_resource) sends no query params, so the request URL will not include vdom=<VDOM>. If the FortiGate API still requires vdom as a query parameter, imports will fail or target the wrong scope. Consider adding vdom=VDOM to the query params for this POST (either by passing additional params here, or by having PostAsJson/GetUrl include VDOM by default).
| PostAsJson(import_certificate_api, cert_resource); | |
| var importCertificateUrl = import_certificate_api; | |
| var separator = importCertificateUrl.Contains("?") ? "&" : "?"; | |
| importCertificateUrl = $"{importCertificateUrl}{separator}vdom={HttpUtility.UrlEncode(VDOM)}"; | |
| PostAsJson(importCertificateUrl, cert_resource); |
| if (response.StatusCode == System.Net.HttpStatusCode.NotFound) | ||
| throw new FortigateException($"VDOM {VDOM} not found."); | ||
| if (!response.IsSuccessStatusCode) | ||
| throw new FortigateException($"Error retrieving VDOM {VDOM}. Status={response.StatusCode.ToString()}, Error={response.Content} {response.ReasonPhrase}"); |
There was a problem hiding this comment.
ValidateVDOM() builds an error message using response.Content, which is an HttpContent object and won’t contain the response body unless you read it. This makes troubleshooting much harder and may log only the type name. Read the content via ReadAsStringAsync() (or similar) and include that string in the exception message.
| if (response.StatusCode == System.Net.HttpStatusCode.NotFound) | |
| throw new FortigateException($"VDOM {VDOM} not found."); | |
| if (!response.IsSuccessStatusCode) | |
| throw new FortigateException($"Error retrieving VDOM {VDOM}. Status={response.StatusCode.ToString()}, Error={response.Content} {response.ReasonPhrase}"); | |
| var content = response.Content.ReadAsStringAsync().GetAwaiter().GetResult(); | |
| if (response.StatusCode == System.Net.HttpStatusCode.NotFound) | |
| throw new FortigateException($"VDOM {VDOM} not found."); | |
| if (!response.IsSuccessStatusCode) | |
| throw new FortigateException($"Error retrieving VDOM {VDOM}. Status={response.StatusCode.ToString()}, Error={content} {response.ReasonPhrase}"); |
| logger.LogDebug($"...End {config.Capability} job for job id {config.JobId}"); | ||
| return new JobResult() { Result = OrchestratorJobStatusJobResult.Success, JobHistoryId = config.JobHistoryId }; | ||
|
|
||
| if (atLeastOneError) | ||
| return new JobResult() { Result = OrchestratorJobStatusJobResult.Warning, JobHistoryId = config.JobHistoryId, FailureMessage = "At least one certificate was unable to be retrieved, Please check the log for more information." }; | ||
| else | ||
| return new JobResult() { Result = OrchestratorJobStatusJobResult.Success, JobHistoryId = config.JobHistoryId }; |
There was a problem hiding this comment.
The FailureMessage text has a grammar/capitalization issue (comma splice and capital “Please” mid-sentence). Consider rephrasing to a single sentence and/or include actionable context (e.g., how many certificates failed, and that details are in logs).
| | Between `11.0.0` and `11.5.1` (inclusive) | `net8.0` | `Disable` | `net6.0` | | ||
| | Between `11.0.0` and `11.5.1` (inclusive) | `net8.0` | `LatestMajor` | `net8.0` | | ||
| | `11.6` _and_ newer | `net8.0` | | `net8.0` | | ||
| | Between `11.0.0` and `11.5.1` (inclusive) | `net8.0` | `Disable` | `net6.0` || Between `11.0.0` and `11.5.1` (inclusive) | `net8.0` | `LatestMajor` | `net8.0` | |
There was a problem hiding this comment.
This Markdown table row contains || and appears to have two rows merged into one line, which breaks table rendering. Split this into two separate table rows so the compatibility matrix renders correctly.
| | Between `11.0.0` and `11.5.1` (inclusive) | `net8.0` | `Disable` | `net6.0` || Between `11.0.0` and `11.5.1` (inclusive) | `net8.0` | `LatestMajor` | `net8.0` | | |
| | Between `11.0.0` and `11.5.1` (inclusive) | `net8.0` | `Disable` | `net6.0` | | |
| | Between `11.0.0` and `11.5.1` (inclusive) | `net8.0` | `LatestMajor` | `net8.0` | |
| 3. Ability to replace bound* and unbound local user certificates (usually after renewal in Keyfactor Command) | ||
| 4. Ability to delete **unbound** local user certificates | ||
| The Fortigate Orchestrator Extension supports the following use cases against a specified VDOM: | ||
| 1. Inventory of local user and factory VDOM and globally scoped cerificates |
There was a problem hiding this comment.
Spelling: cerificates should be certificates.
| 1. Inventory of local user and factory VDOM and globally scoped cerificates | |
| 1. Inventory of local user and factory VDOM and globally scoped certificates |
| The Fortigate Orchestrator Extension supports the following use cases against a specified VDOM: | ||
| 1. Inventory of local user and factory VDOM and globally scoped cerificates | ||
| 2. Ability to add new local VDOM scoped certificates |
There was a problem hiding this comment.
Spelling: cerificates should be certificates.
| elif [ -n "${KEYFACTOR_AUTH_CLIENT_ID}" ] && [ -n "${KEYFACTOR_AUTH_CLIENT_SECRET}" ] && [ -n "${KEYFACTOR_AUTH_TOKEN_URL}" ]; then | ||
| echo "Fetching OAuth token..." | ||
| BEARER_TOKEN=$(curl -s -X POST "${KEYFACTOR_AUTH_TOKEN_URL}" \ | ||
| -H "Content-Type: application/x-www-form-urlencoded" \ | ||
| --data-urlencode "grant_type=client_credentials" \ | ||
| --data-urlencode "client_id=${KEYFACTOR_AUTH_CLIENT_ID}" \ | ||
| --data-urlencode "client_secret=${KEYFACTOR_AUTH_CLIENT_SECRET}" | jq -r '.access_token') | ||
| if [ -z "${BEARER_TOKEN}" ] || [ "${BEARER_TOKEN}" = "null" ]; then | ||
| echo "ERROR: Failed to fetch OAuth token from ${KEYFACTOR_AUTH_TOKEN_URL}" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
This script parses the OAuth token response with jq, but it doesn’t check that jq is installed. Add a dependency check (similar to the existing kfutil check in other scripts) or avoid jq by parsing with POSIX tools, otherwise the script will fail with a confusing error on systems without jq.
| v1.4.0 | ||
| - Add ability to manage custom VDOMs. PLEASE NOTE this release contains a breaking change. Store Path MUST contain the value for the VDOM the certificate will be managing. `root` must be entered to manage the default 'root' VDOM. | ||
|
|
There was a problem hiding this comment.
PR title indicates a 1.4.1 merge, but the changelog entry added here is v1.4.0. If this PR is meant to merge 1.4.1, consider adding/updating the v1.4.1 section (or confirm the PR title/version is correct).
Merge release-1.4 to main - Automated PR