Feature/discovery job#25
Open
bhillkeyfactor wants to merge 22 commits intorelease-1.2from
Open
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>
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
1 participant
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.