FCLI Aviator 25.3.0 Release: User Session Token Validation, Remediation Output, and Command Simplifications #768
Conversation
… config initialization
…factor_aviator_module
kireetivar
changed the title
| } catch (AviatorSimpleException e) { | ||
| throw e; | ||
| } catch (Exception e) { | ||
| LOG.warn("An error occurred while trying to validate the Aviator user token with the server using the 'default' admin config: {}. " + |
There was a problem hiding this comment.
What's the rationale for logging a warning, instead of throwing an exception? Usually (for SSC/ScanCentral/FoD), we throw an exception if connection check fails, which will result in a non-zero exit code to inform the caller process like CI pipeline about this failure.
There was a problem hiding this comment.
This was intended to handle cases like using an older (e.g. 23.2) Aviator server with a newer FCLI version, where validation would always fail but you’re right and i forgot that scenario will actually never happen and I forgot that. I agree I’ll update it to fail fast so it aligns better with our CI/CD expectations.
There was a problem hiding this comment.
@kireetivar I understand, and in fact having these as warnings allowed me to verify the issue with the static field mentioned below (testing these 25.3 updates against 25.2 Aviator service). Backward compatibility with older Aviator backend versions is also something to consider once we allow customers to run their own Aviator service I guess, so certainly something to keep in mind.
When testing this PR, I think the warning showed something like 'endpoint doesn't exist', so apparently we can differentiate that from other error conditions. So, if we'd want to have backward compatibility, we could potentially check for this specific error to identify whether we're talking to Aviator 25.2 or 25.3, and then take the appropriate action (i.e., just ignore this particular error, or output a warning stating that connection check is not supported by current Aviator backend version).
|
|
||
| public static Date extractExpiryDateFromToken(String token) { | ||
| if (token == null) { | ||
| private static JsonNode getJwtPayload(String token) { |
There was a problem hiding this comment.
Similar comment as above; why log a warning instead of throwing an exception in case of issues with the JWT token?
| @@ -42,98 +45,87 @@ public class AviatorSSCAuditCommand extends AbstractSSCJsonNodeOutputCommand imp | |||
| @Option(names = {"--tag-mapping"}, required = false, description = "Tag Mapping") private String tagMapping; | |||
|
|
|||
| private static final Logger LOG = LoggerFactory.getLogger(AviatorSSCAuditCommand.class); | |||
| private String auditAction; | |||
|
|
|||
| private static final ThreadLocal<JsonNode> cachedResult = new ThreadLocal<>(); | |||
There was a problem hiding this comment.
As mentioned before: As per fcli best practices, command classes shouldn’t use instance fields to hold state; only instance fields annotated with Picocli annotations like @Mixin or @Option are allowed. Although not explicitly stated, storing state in static fields is even worse (and ThreadLocal doesn't help); it's not only considered a violation of fcli best practices, but may cause major bugs, especially when used in combination with fcli actions.
As a simple example, see output of the bulkaudit.yaml action developed by @fransvanbuul:
There was a problem hiding this comment.
it seems I misunderstood how to handle state in command classes. Could you please advise me on the best way to dynamically set the action result in AviatorSSCAuditCommand (e.g., SKIPPED, AUDITED) without using instance fields? I've tried a few other approaches, but they don't seem to be working.
There was a problem hiding this comment.
Usually, the getActionCommandResult method should return a fixed value, which will be considered as the default value for the Action column in the table output. Individual records as returned by the getJsonNode method may override this default value for the Action column by setting a JSON property, for example:
result.put(IActionCommandResultSupplier.actionFieldName, "SKIPPED")
This PR contains following changes:
aviator session logincommand to ensure authentication.aviator auditcommand now generates aremediations.xmlfile with code fix suggestions based on audit results.--emailoption inaviator token deleteandaviator token revokecommands, as it is no longer required by the API.--emailoption optional for other token commands (e.g.,aviator token list).Note: Please Do not merge this PR yet