Feature/discovery job#25
Merged
Merged
Conversation
Markdown source and generated PDF describing the Discovery job: before/after comparison, step-by-step flow, store path format, DataPower API endpoints used, and implementation architecture. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
# Conflicts: # README.md
Ports the canonical FlowLogger pattern from barracuda-waf-orchestrator's split-store-types branch and applies the orchestrator hardening checklist. New files: - DataPower/FlowLogger.cs - step-oriented breadcrumb logger appended to JobResult.FailureMessage on both success and failure - DataPower/Client/DataPowerApiException.cs - typed API error carrying HTTP status + response body, with Find() walker for AggregateException - DataPower/Jobs/JobBase.cs - shared plumbing for PAM resolution, JobResult helpers, and exception unwrapping Hardening: - Null/empty argument validation at every public job boundary - Streams disposed via using blocks - DataPowerClient.ApiRequestString throws DataPowerApiException on WebException with status code and trimmed response body - Trace logs mask sensitive payload fields (content, Password, PasswordAlias) before serializing the request body - PAM fields resolved with warn-on-empty fallback - Inventory, Management, and Discovery now derive from JobBase and wrap ProcessJob bodies in using FlowLogger blocks .gitignore additions: .claude/ (per-machine IDE state), .secrets/, *.env Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
GET /mgmt/filestore/{domain} returns filestore.location[] with names like
"cert:" / "pubcert:" / "sharedcert:" — not filestore.directory[]. The response
model deserialized the wrong key, so ListFileStoreDirectories() always returned
an empty list and Discovery submitted 0 store paths against a populated
appliance.
- Rename FileStoreDirectory -> FileStoreLocation (matches JSON shape).
- ListFileStoreResponse: JsonProperty("directory") -> "location",
Directories -> Locations.
- DataPowerClient: single-item-quirk container name "directory" -> "location".
- Discovery: trim trailing ':' before matching against the cert-store filter
so "cert:" matches "cert" and the resulting storePath stays colon-free.
- Update docs/discovery-overview.{md,html} references.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Postman collection + PowerShell generator that produce a populated test appliance for exercising Discovery and Inventory: - generate-test-certs.ps1: emits 3 Collection Runner data files under test/data/ with 120 unique self-signed cert/key pairs (PKCS#8 keys, since DataPower's filestore validator rejects PKCS#1). - DataPower-Test-Setup.postman_collection.json: creates 10 application domains, populates default/pubcert (10 certs), default/sharedcert (10 cert+key pairs + matching CryptoCertificate/CryptoKey config objects in default), and per-domain cert/ (10 cert+key pairs per domain plus a CryptoCertificate and CryptoKey object per pair). Verify folder mirrors what Discovery and Inventory query. Cleanup folder tears it all down. - DataPower-Test.postman_environment.json: BASE_URL / USERNAME / PASSWORD placeholders only — no secrets committed. - README.md: run order, why config objects are required for the per-domain and sharedcert paths (Inventory reads CryptoCertificate objects, not the filestore), and Newman invocation for environments without GUI Runner data-file support. - test/.gitignore excludes data/ since the generated PEMs are throwaway. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
GetCerts and GetPublicCerts both unconditionally called
ci.InventoryBlackList.Split(','), which NRE'd whenever Command sent the
property as JSON null (the common case when the user leaves the optional
"Inventory Black List" field empty during store approval —
DefaultValueHandling.Populate doesn't override an explicit null).
- New ParseInventoryBlacklist helper returns a case-insensitive HashSet,
tolerates null/whitespace, and trims empty entries.
- GetCerts: null-guard viewCertificateCollection?.CryptoCertificates and
null-check items in the loop.
- GetPublicCerts: tighten the
PubFileStoreLocation?.PubFileStore?.PubFiles chain into one local so
any link being null degrades gracefully instead of throwing.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Without enabling Trace-level logging on the orchestrator agent, an empty inventory result tells you nothing about where the certs disappear (parse? blacklist? per-cert detail fetch?). Add two flow steps inside each inventory path so the failure breadcrumb makes the count visible in the standard FlowLogger summary the user already sees: - GetCerts.ParseResponse — certCount=N, blacklistCount=N - GetCerts.SubmitInventory — itemCount=N - GetPublicCerts.ParseResponse — pubFileCount=N, blacklistCount=N - GetPublicCerts.SubmitInventory — itemCount=N Plumb FlowLogger through GetCerts/GetPublicCerts as an optional parameter (defaults to null so internal callers don't break). Existing per-cert trace messages (request/response/loop iteration) already live at LogTrace and are unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The RequestManager constructor was newing up a bare LoggerFactory and asking
it for a logger:
var loggerFactory = (ILoggerFactory) new LoggerFactory();
var reqLogger = loggerFactory.CreateLogger<RequestManager>();
That factory has no providers attached, so every _logger.LogTrace and
_logger.LogError call inside RequestManager (the entire request/response
breadcrumb in GetCerts and GetPublicCerts plus all the management paths)
was silently dropped. The job log shows lots of trace lines from
DataPower.Jobs.Inventory but nothing from DataPower.RequestManager.
Use LogHandler.GetClassLogger<RequestManager>() like the rest of the
codebase (Inventory, Discovery, Management, JobBase). Field type goes from
ILogger<RequestManager> to ILogger to match what LogHandler returns.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When DataPower returns 4xx/5xx, the appliance includes a short JSON message
explaining what's wrong (e.g. "Cannot find requested certificate object",
"Invalid action name"). The client was capturing that body into
DataPowerApiException.ResponseBody but never putting it in the exception
message or the ErrorLog, so callers downstream only saw "HTTP 400 BadRequest"
with no detail.
- ApiRequestString error log now includes the body alongside the status
- DataPowerApiException message now ends with "Body: {body}", so the body
appears wherever the exception's Message is rendered (per-cert
"Certificate not retrievable" log lines, etc.)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
DataPower's CryptoCertificate loader rejected our barebones self-signed certs with "The specified certificate has an unreadable, corrupt, or invalid certificate file or has an invalid password." The certs parsed fine with openssl, but DataPower expects a real end-entity TLS cert. Add the standard extensions a normal server cert carries: - BasicConstraints (cA=false, critical) - KeyUsage (DigitalSignature + KeyEncipherment, critical) - ExtendedKeyUsage (serverAuth, clientAuth) - SubjectKeyIdentifier Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
GET /mgmt/config/{domain}/CryptoCertificate returns every CryptoCertificate
object in the domain, regardless of whether its Filename points at
cert:///, pubcert:///, or sharedcert:///. GetCerts iterated over all of
them, so an inventory job for "default\sharedcert" surfaced pubcert: items
too (DigicertRoot, Goog showing up alongside sharedcert entries).
Filter the list to objects whose Filename starts with "{ci.CertificateStore}:"
so the inventory only contains items that actually live in the requested
store. The FlowLogger ParseResponse step now reports both totals:
GetCerts.ParseResponse - certCount=4 (filtered from 6 by scheme 'sharedcert:'), blacklistCount=0
so it's obvious when filtering is dropping items.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The cert-store directory filter was a hardcoded constant
({cert, pubcert, sharedcert}) and the Discovery form's "Directories to
search" value was silently ignored. Operators with custom DataPower
filestore schemes had no way to extend it, and a user could fill in the
field expecting it to scope the run.
Read the comma-separated value from JobProperties (trying common key
casings: dirs, Dirs, directories, Directories, DirsToSearch). Trim,
strip any trailing colon, dedupe (case-insensitive). Fall back to the
standard {cert, pubcert, sharedcert} set when the field is empty or
absent. The FlowLogger summary now exposes which list was applied:
[OK] ResolveDirsToSearch - source=user (key=dirs), dirs=[cert,sharedcert]
[OK] ResolveDirsToSearch - source=default, dirs=[cert,pubcert,sharedcert]
docs/discovery-overview.{md,html} updated to describe the field.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three connected fixes that share a root cause: pubcert and sharedcert are
appliance-wide on DataPower (owned by the default domain) but the code paths
treated them like per-domain stores. Discovery emitted N copies (one per
domain) all aliasing the same physical data; Add tried to PUT through the
per-domain endpoint and got HTTP 403; the catch in AddCertStore swallowed
the 403 and returned Success, so Command thought the cert was added when
nothing was on the appliance.
- Discovery: emit appliance-wide directories (pubcert, sharedcert) only
under "default", not once per domain. Per-domain "cert" still emitted
for every domain. A 10-domain appliance now produces 12 store paths
(10 per-domain + default\pubcert + default\sharedcert) instead of 30.
- AddCertStore: add a sharedcert guard mirroring the existing pubcert
guard. Adds against <non-default>\sharedcert return Failure with
"You can only add to sharedcert on the default domain" instead of
letting the appliance's 403 bubble up.
- AddCertStore catch: was logging Trace, calling SaveConfig (persisting
whatever partial state the appliance reached when the Add blew up),
and falling through to return Success. Now: log Error with the full
exception, extract the appliance's response body via
DataPowerApiException.Find when present, and return Failure with the
body in the FailureMessage so operators see what DataPower actually
rejected.
- docs/discovery-overview.{md,html}: describe the new emit shape, the
per-domain vs appliance-wide split, and a migration note for existing
deployments that have already approved <non-default>\pubcert /
<non-default>\sharedcert as cert stores - those are now orphans and
should be deleted in favor of default\pubcert / default\sharedcert.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous readme_source.md and docsource/content.md duplicated content
that the doctool generates from integration-manifest.json (Compatibility,
Support, Requirements & Prerequisites, the per-store Manual Creation
Basic/Advanced/Custom Fields tables). When the shared
keyfactor-starter-workflow.yml regenerates README.md, that produced
duplicated headings and stale tables. docsource also carried a
fortiweb.md file from a prior copy-paste that doesn't belong in this
repo.
- readme_source.md: trimmed to ~25 lines. Short overview, the
vendor-side prereqs (REST mgmt enabled on 5554, RBAC for the API
user, network reachability), and License. No more inline
store-type-config tables, no more test-case dumps - those are either
manifest-derived or development scratch.
- docsource/content.md: rebuilt as the architectural overview. Mermaid
flow, store-path format, the per-domain vs appliance-wide split with
the new emit shape (12 paths for a 10-domain appliance, not 30),
Discovery walkthrough including the "Directories to search" knob and
the FlowLogger source=user|default line, Inventory + Management
branching summary, optional store properties description, and a
migration note for deployments that already approved
<non-default>\pubcert / <non-default>\sharedcert as orphan stores.
Removed the Requirements section (doctool generates it) and the
test-case tables (development checklist, not customer-facing).
- docsource/datapower.md: rebuilt as the per-store-type doc that
doctool injects inside the store's <details> block. Purpose header
(with the cert / pubcert / sharedcert business-mapping table),
prereqs specific to this store type (REST mgmt CLI snippet,
curl-based access probe), operational notes (FlowLogger summary
entries to watch for), and a Common Errors table covering the 403
on non-default sharedcert, the silent-failure surface, and the
"unreadable certificate file" parser rejection. No Store Type
Settings / Custom Fields tables - doctool generates those from
Properties[].
- docsource/fortiweb.md: deleted. Wrong vendor; leftover.
The standalone docs/discovery-overview.{md,html,pdf} are kept as
internal feature-spec reference - they're not part of the doctool
pipeline.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
indrora
added a commit
that referenced
this pull request
May 6, 2026
* Datapower Discovery Job
* Update generated docs
* fixed path documentation
* Update generated docs
* Add Discovery feature spec documentation
Markdown source and generated PDF describing the Discovery job:
before/after comparison, step-by-step flow, store path format,
DataPower API endpoints used, and implementation architecture.
* Add FlowLogger and hardening across all jobs
Ports the canonical FlowLogger pattern from barracuda-waf-orchestrator's
split-store-types branch and applies the orchestrator hardening checklist.
New files:
- DataPower/FlowLogger.cs - step-oriented breadcrumb logger appended to
JobResult.FailureMessage on both success and failure
- DataPower/Client/DataPowerApiException.cs - typed API error carrying
HTTP status + response body, with Find() walker for AggregateException
- DataPower/Jobs/JobBase.cs - shared plumbing for PAM resolution,
JobResult helpers, and exception unwrapping
Hardening:
- Null/empty argument validation at every public job boundary
- Streams disposed via using blocks
- DataPowerClient.ApiRequestString throws DataPowerApiException on
WebException with status code and trimmed response body
- Trace logs mask sensitive payload fields (content, Password,
PasswordAlias) before serializing the request body
- PAM fields resolved with warn-on-empty fallback
- Inventory, Management, and Discovery now derive from JobBase and wrap
ProcessJob bodies in using FlowLogger blocks
.gitignore additions: .claude/ (per-machine IDE state), .secrets/, *.env
* Update keyfactor-starter-workflow.yml
* Fix Discovery: parse filestore.location[] and strip trailing colon
GET /mgmt/filestore/{domain} returns filestore.location[] with names like
"cert:" / "pubcert:" / "sharedcert:" — not filestore.directory[]. The response
model deserialized the wrong key, so ListFileStoreDirectories() always returned
an empty list and Discovery submitted 0 store paths against a populated
appliance.
- Rename FileStoreDirectory -> FileStoreLocation (matches JSON shape).
- ListFileStoreResponse: JsonProperty("directory") -> "location",
Directories -> Locations.
- DataPowerClient: single-item-quirk container name "directory" -> "location".
- Discovery: trim trailing ':' before matching against the cert-store filter
so "cert:" matches "cert" and the resulting storePath stays colon-free.
- Update docs/discovery-overview.{md,html} references.
* Add test setup for populating a DataPower lab appliance
Postman collection + PowerShell generator that produce a populated test
appliance for exercising Discovery and Inventory:
- generate-test-certs.ps1: emits 3 Collection Runner data files under test/data/
with 120 unique self-signed cert/key pairs (PKCS#8 keys, since DataPower's
filestore validator rejects PKCS#1).
- DataPower-Test-Setup.postman_collection.json: creates 10 application
domains, populates default/pubcert (10 certs), default/sharedcert
(10 cert+key pairs + matching CryptoCertificate/CryptoKey config objects
in default), and per-domain cert/ (10 cert+key pairs per domain plus a
CryptoCertificate and CryptoKey object per pair). Verify folder mirrors
what Discovery and Inventory query. Cleanup folder tears it all down.
- DataPower-Test.postman_environment.json: BASE_URL / USERNAME / PASSWORD
placeholders only — no secrets committed.
- README.md: run order, why config objects are required for the per-domain
and sharedcert paths (Inventory reads CryptoCertificate objects, not the
filestore), and Newman invocation for environments without GUI Runner
data-file support.
- test/.gitignore excludes data/ since the generated PEMs are throwaway.
* Harden Inventory paths against NullReferenceException
GetCerts and GetPublicCerts both unconditionally called
ci.InventoryBlackList.Split(','), which NRE'd whenever Command sent the
property as JSON null (the common case when the user leaves the optional
"Inventory Black List" field empty during store approval —
DefaultValueHandling.Populate doesn't override an explicit null).
- New ParseInventoryBlacklist helper returns a case-insensitive HashSet,
tolerates null/whitespace, and trims empty entries.
- GetCerts: null-guard viewCertificateCollection?.CryptoCertificates and
null-check items in the loop.
- GetPublicCerts: tighten the
PubFileStoreLocation?.PubFileStore?.PubFiles chain into one local so
any link being null degrades gracefully instead of throwing.
* Surface inventory counts in FlowLogger summary
Without enabling Trace-level logging on the orchestrator agent, an empty
inventory result tells you nothing about where the certs disappear (parse?
blacklist? per-cert detail fetch?). Add two flow steps inside each inventory
path so the failure breadcrumb makes the count visible in the standard
FlowLogger summary the user already sees:
- GetCerts.ParseResponse — certCount=N, blacklistCount=N
- GetCerts.SubmitInventory — itemCount=N
- GetPublicCerts.ParseResponse — pubFileCount=N, blacklistCount=N
- GetPublicCerts.SubmitInventory — itemCount=N
Plumb FlowLogger through GetCerts/GetPublicCerts as an optional parameter
(defaults to null so internal callers don't break). Existing per-cert trace
messages (request/response/loop iteration) already live at LogTrace and are
unchanged.
* Wire RequestManager logger into the orchestrator's NLog pipeline
The RequestManager constructor was newing up a bare LoggerFactory and asking
it for a logger:
var loggerFactory = (ILoggerFactory) new LoggerFactory();
var reqLogger = loggerFactory.CreateLogger<RequestManager>();
That factory has no providers attached, so every _logger.LogTrace and
_logger.LogError call inside RequestManager (the entire request/response
breadcrumb in GetCerts and GetPublicCerts plus all the management paths)
was silently dropped. The job log shows lots of trace lines from
DataPower.Jobs.Inventory but nothing from DataPower.RequestManager.
Use LogHandler.GetClassLogger<RequestManager>() like the rest of the
codebase (Inventory, Discovery, Management, JobBase). Field type goes from
ILogger<RequestManager> to ILogger to match what LogHandler returns.
* Surface DataPower error response body in API call failures
When DataPower returns 4xx/5xx, the appliance includes a short JSON message
explaining what's wrong (e.g. "Cannot find requested certificate object",
"Invalid action name"). The client was capturing that body into
DataPowerApiException.ResponseBody but never putting it in the exception
message or the ErrorLog, so callers downstream only saw "HTTP 400 BadRequest"
with no detail.
- ApiRequestString error log now includes the body alongside the status
- DataPowerApiException message now ends with "Body: {body}", so the body
appears wherever the exception's Message is rendered (per-cert
"Certificate not retrievable" log lines, etc.)
* Add x509 extensions to generated test certs
DataPower's CryptoCertificate loader rejected our barebones self-signed
certs with "The specified certificate has an unreadable, corrupt, or
invalid certificate file or has an invalid password." The certs parsed
fine with openssl, but DataPower expects a real end-entity TLS cert.
Add the standard extensions a normal server cert carries:
- BasicConstraints (cA=false, critical)
- KeyUsage (DigitalSignature + KeyEncipherment, critical)
- ExtendedKeyUsage (serverAuth, clientAuth)
- SubjectKeyIdentifier
* Filter CryptoCertificate inventory by store path's URI scheme
GET /mgmt/config/{domain}/CryptoCertificate returns every CryptoCertificate
object in the domain, regardless of whether its Filename points at
cert:///, pubcert:///, or sharedcert:///. GetCerts iterated over all of
them, so an inventory job for "default\sharedcert" surfaced pubcert: items
too (DigicertRoot, Goog showing up alongside sharedcert entries).
Filter the list to objects whose Filename starts with "{ci.CertificateStore}:"
so the inventory only contains items that actually live in the requested
store. The FlowLogger ParseResponse step now reports both totals:
GetCerts.ParseResponse - certCount=4 (filtered from 6 by scheme 'sharedcert:'), blacklistCount=0
so it's obvious when filtering is dropping items.
* Discovery: honor "Directories to search" job field
The cert-store directory filter was a hardcoded constant
({cert, pubcert, sharedcert}) and the Discovery form's "Directories to
search" value was silently ignored. Operators with custom DataPower
filestore schemes had no way to extend it, and a user could fill in the
field expecting it to scope the run.
Read the comma-separated value from JobProperties (trying common key
casings: dirs, Dirs, directories, Directories, DirsToSearch). Trim,
strip any trailing colon, dedupe (case-insensitive). Fall back to the
standard {cert, pubcert, sharedcert} set when the field is empty or
absent. The FlowLogger summary now exposes which list was applied:
[OK] ResolveDirsToSearch - source=user (key=dirs), dirs=[cert,sharedcert]
[OK] ResolveDirsToSearch - source=default, dirs=[cert,pubcert,sharedcert]
docs/discovery-overview.{md,html} updated to describe the field.
* Coherent appliance-wide store handling for pubcert/sharedcert
Three connected fixes that share a root cause: pubcert and sharedcert are
appliance-wide on DataPower (owned by the default domain) but the code paths
treated them like per-domain stores. Discovery emitted N copies (one per
domain) all aliasing the same physical data; Add tried to PUT through the
per-domain endpoint and got HTTP 403; the catch in AddCertStore swallowed
the 403 and returned Success, so Command thought the cert was added when
nothing was on the appliance.
- Discovery: emit appliance-wide directories (pubcert, sharedcert) only
under "default", not once per domain. Per-domain "cert" still emitted
for every domain. A 10-domain appliance now produces 12 store paths
(10 per-domain + default\pubcert + default\sharedcert) instead of 30.
- AddCertStore: add a sharedcert guard mirroring the existing pubcert
guard. Adds against <non-default>\sharedcert return Failure with
"You can only add to sharedcert on the default domain" instead of
letting the appliance's 403 bubble up.
- AddCertStore catch: was logging Trace, calling SaveConfig (persisting
whatever partial state the appliance reached when the Add blew up),
and falling through to return Success. Now: log Error with the full
exception, extract the appliance's response body via
DataPowerApiException.Find when present, and return Failure with the
body in the FailureMessage so operators see what DataPower actually
rejected.
- docs/discovery-overview.{md,html}: describe the new emit shape, the
per-domain vs appliance-wide split, and a migration note for existing
deployments that have already approved <non-default>\pubcert /
<non-default>\sharedcert as cert stores - those are now orphans and
should be deleted in favor of default\pubcert / default\sharedcert.
* Delete docsource/fortiweb.md
* Restructure docs to match doctool docsource contract
The previous readme_source.md and docsource/content.md duplicated content
that the doctool generates from integration-manifest.json (Compatibility,
Support, Requirements & Prerequisites, the per-store Manual Creation
Basic/Advanced/Custom Fields tables). When the shared
keyfactor-starter-workflow.yml regenerates README.md, that produced
duplicated headings and stale tables. docsource also carried a
fortiweb.md file from a prior copy-paste that doesn't belong in this
repo.
- readme_source.md: trimmed to ~25 lines. Short overview, the
vendor-side prereqs (REST mgmt enabled on 5554, RBAC for the API
user, network reachability), and License. No more inline
store-type-config tables, no more test-case dumps - those are either
manifest-derived or development scratch.
- docsource/content.md: rebuilt as the architectural overview. Mermaid
flow, store-path format, the per-domain vs appliance-wide split with
the new emit shape (12 paths for a 10-domain appliance, not 30),
Discovery walkthrough including the "Directories to search" knob and
the FlowLogger source=user|default line, Inventory + Management
branching summary, optional store properties description, and a
migration note for deployments that already approved
<non-default>\pubcert / <non-default>\sharedcert as orphan stores.
Removed the Requirements section (doctool generates it) and the
test-case tables (development checklist, not customer-facing).
- docsource/datapower.md: rebuilt as the per-store-type doc that
doctool injects inside the store's <details> block. Purpose header
(with the cert / pubcert / sharedcert business-mapping table),
prereqs specific to this store type (REST mgmt CLI snippet,
curl-based access probe), operational notes (FlowLogger summary
entries to watch for), and a Common Errors table covering the 403
on non-default sharedcert, the silent-failure surface, and the
"unreadable certificate file" parser rejection. No Store Type
Settings / Custom Fields tables - doctool generates those from
Properties[].
- docsource/fortiweb.md: deleted. Wrong vendor; leftover.
The standalone docs/discovery-overview.{md,html,pdf} are kept as
internal feature-spec reference - they're not part of the doctool
pipeline.
* Update generated docs
* Delete .claude directory
---------
Co-authored-by: Brian Hill <76450501+bhillkeyfactor@users.noreply.github.com>
Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
indrora
added a commit
that referenced
this pull request
May 21, 2026
* Feature/discovery job (#25) * Datapower Discovery Job * Update generated docs * fixed path documentation * Update generated docs * Add Discovery feature spec documentation Markdown source and generated PDF describing the Discovery job: before/after comparison, step-by-step flow, store path format, DataPower API endpoints used, and implementation architecture. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Add FlowLogger and hardening across all jobs Ports the canonical FlowLogger pattern from barracuda-waf-orchestrator's split-store-types branch and applies the orchestrator hardening checklist. New files: - DataPower/FlowLogger.cs - step-oriented breadcrumb logger appended to JobResult.FailureMessage on both success and failure - DataPower/Client/DataPowerApiException.cs - typed API error carrying HTTP status + response body, with Find() walker for AggregateException - DataPower/Jobs/JobBase.cs - shared plumbing for PAM resolution, JobResult helpers, and exception unwrapping Hardening: - Null/empty argument validation at every public job boundary - Streams disposed via using blocks - DataPowerClient.ApiRequestString throws DataPowerApiException on WebException with status code and trimmed response body - Trace logs mask sensitive payload fields (content, Password, PasswordAlias) before serializing the request body - PAM fields resolved with warn-on-empty fallback - Inventory, Management, and Discovery now derive from JobBase and wrap ProcessJob bodies in using FlowLogger blocks .gitignore additions: .claude/ (per-machine IDE state), .secrets/, *.env Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Update keyfactor-starter-workflow.yml * Fix Discovery: parse filestore.location[] and strip trailing colon GET /mgmt/filestore/{domain} returns filestore.location[] with names like "cert:" / "pubcert:" / "sharedcert:" — not filestore.directory[]. The response model deserialized the wrong key, so ListFileStoreDirectories() always returned an empty list and Discovery submitted 0 store paths against a populated appliance. - Rename FileStoreDirectory -> FileStoreLocation (matches JSON shape). - ListFileStoreResponse: JsonProperty("directory") -> "location", Directories -> Locations. - DataPowerClient: single-item-quirk container name "directory" -> "location". - Discovery: trim trailing ':' before matching against the cert-store filter so "cert:" matches "cert" and the resulting storePath stays colon-free. - Update docs/discovery-overview.{md,html} references. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Add test setup for populating a DataPower lab appliance Postman collection + PowerShell generator that produce a populated test appliance for exercising Discovery and Inventory: - generate-test-certs.ps1: emits 3 Collection Runner data files under test/data/ with 120 unique self-signed cert/key pairs (PKCS#8 keys, since DataPower's filestore validator rejects PKCS#1). - DataPower-Test-Setup.postman_collection.json: creates 10 application domains, populates default/pubcert (10 certs), default/sharedcert (10 cert+key pairs + matching CryptoCertificate/CryptoKey config objects in default), and per-domain cert/ (10 cert+key pairs per domain plus a CryptoCertificate and CryptoKey object per pair). Verify folder mirrors what Discovery and Inventory query. Cleanup folder tears it all down. - DataPower-Test.postman_environment.json: BASE_URL / USERNAME / PASSWORD placeholders only — no secrets committed. - README.md: run order, why config objects are required for the per-domain and sharedcert paths (Inventory reads CryptoCertificate objects, not the filestore), and Newman invocation for environments without GUI Runner data-file support. - test/.gitignore excludes data/ since the generated PEMs are throwaway. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Harden Inventory paths against NullReferenceException GetCerts and GetPublicCerts both unconditionally called ci.InventoryBlackList.Split(','), which NRE'd whenever Command sent the property as JSON null (the common case when the user leaves the optional "Inventory Black List" field empty during store approval — DefaultValueHandling.Populate doesn't override an explicit null). - New ParseInventoryBlacklist helper returns a case-insensitive HashSet, tolerates null/whitespace, and trims empty entries. - GetCerts: null-guard viewCertificateCollection?.CryptoCertificates and null-check items in the loop. - GetPublicCerts: tighten the PubFileStoreLocation?.PubFileStore?.PubFiles chain into one local so any link being null degrades gracefully instead of throwing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Surface inventory counts in FlowLogger summary Without enabling Trace-level logging on the orchestrator agent, an empty inventory result tells you nothing about where the certs disappear (parse? blacklist? per-cert detail fetch?). Add two flow steps inside each inventory path so the failure breadcrumb makes the count visible in the standard FlowLogger summary the user already sees: - GetCerts.ParseResponse — certCount=N, blacklistCount=N - GetCerts.SubmitInventory — itemCount=N - GetPublicCerts.ParseResponse — pubFileCount=N, blacklistCount=N - GetPublicCerts.SubmitInventory — itemCount=N Plumb FlowLogger through GetCerts/GetPublicCerts as an optional parameter (defaults to null so internal callers don't break). Existing per-cert trace messages (request/response/loop iteration) already live at LogTrace and are unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Wire RequestManager logger into the orchestrator's NLog pipeline The RequestManager constructor was newing up a bare LoggerFactory and asking it for a logger: var loggerFactory = (ILoggerFactory) new LoggerFactory(); var reqLogger = loggerFactory.CreateLogger<RequestManager>(); That factory has no providers attached, so every _logger.LogTrace and _logger.LogError call inside RequestManager (the entire request/response breadcrumb in GetCerts and GetPublicCerts plus all the management paths) was silently dropped. The job log shows lots of trace lines from DataPower.Jobs.Inventory but nothing from DataPower.RequestManager. Use LogHandler.GetClassLogger<RequestManager>() like the rest of the codebase (Inventory, Discovery, Management, JobBase). Field type goes from ILogger<RequestManager> to ILogger to match what LogHandler returns. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Surface DataPower error response body in API call failures When DataPower returns 4xx/5xx, the appliance includes a short JSON message explaining what's wrong (e.g. "Cannot find requested certificate object", "Invalid action name"). The client was capturing that body into DataPowerApiException.ResponseBody but never putting it in the exception message or the ErrorLog, so callers downstream only saw "HTTP 400 BadRequest" with no detail. - ApiRequestString error log now includes the body alongside the status - DataPowerApiException message now ends with "Body: {body}", so the body appears wherever the exception's Message is rendered (per-cert "Certificate not retrievable" log lines, etc.) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Add x509 extensions to generated test certs DataPower's CryptoCertificate loader rejected our barebones self-signed certs with "The specified certificate has an unreadable, corrupt, or invalid certificate file or has an invalid password." The certs parsed fine with openssl, but DataPower expects a real end-entity TLS cert. Add the standard extensions a normal server cert carries: - BasicConstraints (cA=false, critical) - KeyUsage (DigitalSignature + KeyEncipherment, critical) - ExtendedKeyUsage (serverAuth, clientAuth) - SubjectKeyIdentifier Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Filter CryptoCertificate inventory by store path's URI scheme GET /mgmt/config/{domain}/CryptoCertificate returns every CryptoCertificate object in the domain, regardless of whether its Filename points at cert:///, pubcert:///, or sharedcert:///. GetCerts iterated over all of them, so an inventory job for "default\sharedcert" surfaced pubcert: items too (DigicertRoot, Goog showing up alongside sharedcert entries). Filter the list to objects whose Filename starts with "{ci.CertificateStore}:" so the inventory only contains items that actually live in the requested store. The FlowLogger ParseResponse step now reports both totals: GetCerts.ParseResponse - certCount=4 (filtered from 6 by scheme 'sharedcert:'), blacklistCount=0 so it's obvious when filtering is dropping items. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Discovery: honor "Directories to search" job field The cert-store directory filter was a hardcoded constant ({cert, pubcert, sharedcert}) and the Discovery form's "Directories to search" value was silently ignored. Operators with custom DataPower filestore schemes had no way to extend it, and a user could fill in the field expecting it to scope the run. Read the comma-separated value from JobProperties (trying common key casings: dirs, Dirs, directories, Directories, DirsToSearch). Trim, strip any trailing colon, dedupe (case-insensitive). Fall back to the standard {cert, pubcert, sharedcert} set when the field is empty or absent. The FlowLogger summary now exposes which list was applied: [OK] ResolveDirsToSearch - source=user (key=dirs), dirs=[cert,sharedcert] [OK] ResolveDirsToSearch - source=default, dirs=[cert,pubcert,sharedcert] docs/discovery-overview.{md,html} updated to describe the field. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Coherent appliance-wide store handling for pubcert/sharedcert Three connected fixes that share a root cause: pubcert and sharedcert are appliance-wide on DataPower (owned by the default domain) but the code paths treated them like per-domain stores. Discovery emitted N copies (one per domain) all aliasing the same physical data; Add tried to PUT through the per-domain endpoint and got HTTP 403; the catch in AddCertStore swallowed the 403 and returned Success, so Command thought the cert was added when nothing was on the appliance. - Discovery: emit appliance-wide directories (pubcert, sharedcert) only under "default", not once per domain. Per-domain "cert" still emitted for every domain. A 10-domain appliance now produces 12 store paths (10 per-domain + default\pubcert + default\sharedcert) instead of 30. - AddCertStore: add a sharedcert guard mirroring the existing pubcert guard. Adds against <non-default>\sharedcert return Failure with "You can only add to sharedcert on the default domain" instead of letting the appliance's 403 bubble up. - AddCertStore catch: was logging Trace, calling SaveConfig (persisting whatever partial state the appliance reached when the Add blew up), and falling through to return Success. Now: log Error with the full exception, extract the appliance's response body via DataPowerApiException.Find when present, and return Failure with the body in the FailureMessage so operators see what DataPower actually rejected. - docs/discovery-overview.{md,html}: describe the new emit shape, the per-domain vs appliance-wide split, and a migration note for existing deployments that have already approved <non-default>\pubcert / <non-default>\sharedcert as cert stores - those are now orphans and should be deleted in favor of default\pubcert / default\sharedcert. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Delete docsource/fortiweb.md * Restructure docs to match doctool docsource contract The previous readme_source.md and docsource/content.md duplicated content that the doctool generates from integration-manifest.json (Compatibility, Support, Requirements & Prerequisites, the per-store Manual Creation Basic/Advanced/Custom Fields tables). When the shared keyfactor-starter-workflow.yml regenerates README.md, that produced duplicated headings and stale tables. docsource also carried a fortiweb.md file from a prior copy-paste that doesn't belong in this repo. - readme_source.md: trimmed to ~25 lines. Short overview, the vendor-side prereqs (REST mgmt enabled on 5554, RBAC for the API user, network reachability), and License. No more inline store-type-config tables, no more test-case dumps - those are either manifest-derived or development scratch. - docsource/content.md: rebuilt as the architectural overview. Mermaid flow, store-path format, the per-domain vs appliance-wide split with the new emit shape (12 paths for a 10-domain appliance, not 30), Discovery walkthrough including the "Directories to search" knob and the FlowLogger source=user|default line, Inventory + Management branching summary, optional store properties description, and a migration note for deployments that already approved <non-default>\pubcert / <non-default>\sharedcert as orphan stores. Removed the Requirements section (doctool generates it) and the test-case tables (development checklist, not customer-facing). - docsource/datapower.md: rebuilt as the per-store-type doc that doctool injects inside the store's <details> block. Purpose header (with the cert / pubcert / sharedcert business-mapping table), prereqs specific to this store type (REST mgmt CLI snippet, curl-based access probe), operational notes (FlowLogger summary entries to watch for), and a Common Errors table covering the 403 on non-default sharedcert, the silent-failure surface, and the "unreadable certificate file" parser rejection. No Store Type Settings / Custom Fields tables - doctool generates those from Properties[]. - docsource/fortiweb.md: deleted. Wrong vendor; leftover. The standalone docs/discovery-overview.{md,html,pdf} are kept as internal feature-spec reference - they're not part of the doctool pipeline. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Update generated docs * Delete .claude directory --------- Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Bound FlowLogger summary length and aggregate per-domain failures (1.2.1) (#27) * Bound FlowLogger summary length and aggregate per-domain failures Discovery against an appliance with 235 domains produced a 50+ KB FlowLogger summary - two lines per failed domain ([FAIL] ListFileStore-X followed by [SKIP] Domain-X, both echoing the full HTTP 401 body). When the orchestrator returned that as JobResult.FailureMessage, Command's SQL update against AgentJobStatus.Message failed with "String or binary data would be truncated" and the entire job result was lost. The trace showed the job ran fine, but the customer saw a failed job. Two changes: - FlowLogger.GetSummary() now caps output at 3500 chars and appends a "[truncated, N chars omitted; check orchestrator log for full breadcrumb]" marker. Defense in depth - even if a future job somehow blows up the summary again, the result update won't fail. Trace log still carries the full picture. - Discovery aggregates per-domain failures by error signature instead of emitting one FAIL + one SKIP per failed domain. Identical 401s across N domains collapse into one [SKIP] DomainsFailed[HTTP 401 Unauthorized: Authentication failure.] - 114 domain(s): AashishSandbox, ADT_*, ADT_*, ADT_*, ADT_* (+109 more). The /_links self URL (which varies per domain) is stripped from the group key so the grouping actually works. Successful domains still emit a per-path Discovered-X step so operators can see what was found. Net effect on the user's 235-domain appliance: summary drops from ~50 KB / 235+ step lines to ~1 KB / ~10 step lines. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Add 1.2.1 CHANGELOG entry for FlowLogger truncation fix --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Brian Hill <76450501+bhillkeyfactor@users.noreply.github.com> Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.