Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for authentication logging enrichment #1297

Merged
merged 4 commits into from
Jun 19, 2024

Conversation

Jong-Vincent
Copy link
Contributor

Hi all,

With these changes we would like to contribute support for enriching the authentication logging. For some context, this refers to the logging done with 'login granted' where information such as timestamp, SP, IdP etc. is logged. These changes allow the configuration of a mapping in parameters.yml to enrich the logging information with response attributes.

The mapping consists of a label and the name of attribute that is desired e.g. in parameters.yml:

auth.log.attributes: 
    myLabel1: attributeKey1
    myLabel2: attributeKey2
    ...

Default value is empty array.

The logging would then look like

authentication.INFO: login granted {"login_stamp":"2024-04-26T12:02:42.904740+02:00","user_id":"","sp_entity_id":"","idp_entity_id":"","key_id":null,"proxied_sp_entity_ids":[],"workflow_state":"prodaccepted","original_name_id":"3d264e07006590d060e96c618e8fafc13a6d21a1","authncontextclassref":"urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport","requestedidps":[],"engine_sso_endpoint_used":"https://engine/authentication/idp/single-sign-on", "myLabel1":"value1","myLabel2":"value2"} {"session_id":"0gd53uec0mc4g1ohcneq6mo6s6","request_id":"662b7bb806f3e"}

# When a user successfully authenticates and additional logging to the authentication log is desired, the following
# parameter can be used to define a mapping of attributes that will be used to enrich the authentication log.
# A (list) mapping is for example <attributeLabel>: <attributeName> where the label represents the label that is
# used in the authentication log record. The attributeName will be searched in the response attributes and if present
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe clarify what "response attributes" means here. Is it the final attribute value after ARPs, Manipulation and such?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is indeed after ARP and Manipulation, I have added it to the description

@thijskh
Copy link
Member

thijskh commented May 8, 2024

I like the idea, it's a straightforward and yet flexible approach to allow deployers to log something that is specific to their interest.

@thijskh thijskh requested a review from MKodde May 8, 2024 19:09
Copy link
Member

@MKodde MKodde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good. I agree with Thijs that the parameters.yaml.dist documentation could be specified a bit!

$logAttributes = [];
if (!empty($this->configuredLogAttributes)) {
foreach ($this->configuredLogAttributes as $attributeKey => $attributeValue) {
if (!empty($this->_responseAttributes[$attributeValue])) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered a (arguably) more expressive array_key_exists here? I assume the value is always set when the key exists.

Copy link
Contributor Author

@Jong-Vincent Jong-Vincent May 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, implemented the change and did a little refactoring to the variables as well

'engine_sso_endpoint_used' => $engineSsoEndpointUsed,
];
if (!empty($logAttributes)) {
$logData = array_merge($logData, $logAttributes);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The attributes now end up as $logData entries. From a log-audit perspective it might be more 'friendly' to bundle them in a new array.

 $logData = [
        'login_stamp' => $timestamp,
....
        'engine_sso_endpoint_used' => $engineSsoEndpointUsed,
        'response_attributes' => $logAttributes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented the change, thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it's always added even when empty. Maybe better to only add the key if the value is non-empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated this part :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent!

@MKodde MKodde merged commit 9b873b3 into OpenConext:master Jun 19, 2024
2 checks passed
@Jong-Vincent Jong-Vincent deleted the logging_enrichment branch July 2, 2024 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants