Protocols report

[vrajmohan]

🎫 Ticket

Link to the relevant ticket:
https://gitlab.login.gov/lg-people/lg-people-appdev/protocols/common/-/issues/1

🛠 Summary of changes

Added a CloudWatch report of:

1. authentication requests by protocol
2. SAML signatures - missing and invalid

📜 Testing Plan

• Run the report using aws-vault exec $DEPLOYED_ENVIRONMENT-$LOGIN_IAM_PROFILE -- bundle exec rails runner lib/reporting/protocols_report.rb --date=$SOMEDATE.

• Confirm the validity of the reported metrics by querying CloudWatch logs from the AWS console

[zachmargolis]

I would consider formatting this for shorter lines for easier readability, also using consistent casing for keywords like AS

Suggested change

	format(<<~QUERY, params)
	fields name as protocol, properties.event_properties.service_provider as issuer, properties.event_properties.request_signed = 1 AS signed, properties.event_properties.request_signed != 1 AS not_signed, isempty(properties.event_properties.matching_cert_serial) and signed as invalid_signature
	| filter name IN %{event}
	| stats count(*) as request_count, sum(signed) as signed_count, sum(not_signed) as unsigned_count, sum(invalid_signature) as invalid_signature_count by protocol, issuer
	QUERY
	format(<<~QUERY, params)
	fields
	name AS protocol
	, properties.event_properties.service_provider AS issuer
	, properties.event_properties.request_signed = 1 AS signed
	, properties.event_properties.request_signed != 1 AS not_signed
	, isempty(properties.event_properties.matching_cert_serial) AND signed AS invalid_signature
	| filter name IN %{event}
	| stats
	count(*) AS request_count
	, sum(signed) AS signed_count
	, sum(not_signed) AS unsigned_count
	, sum(invalid_signature) AS invalid_signature_count
	by
	protocol
	, issuer
	QUERY

[vrajmohan]

Personally, I don't like the leading commas - it hurts my eyes :-). However, if it is a standard here, I am happy to adopt it.

[zachmargolis]

Not 100% consistent but we do have many files formatted that way:

identity-idp> git grep -l  "  , " | wc -l
      18

[Sgtpluck]

as mentioned in some of our standups, i think this report will end up growing beyond these initial needs, that will require different fields/manipulation of the events. so i was originally envisioning this to be a query for the events with some specific fields that are then manipulated in the data method (example: https://github.com/18F/identity-idp/blob/main/lib/reporting/drop_off_report.rb#L410-L421). so this will probably change in the future, but i think it's fine for now.

[zachmargolis]

Another option is also to have multiple separate queries that get combined in one email or something

[vrajmohan]

I have done that by pulling out the LOA ACR metrics that was also asked for in this ticket into a separate ticket (and consequently separate report). I do see requests by protocols and SAML signature issues as being separate concerns. I didn't want to do too much given the Data Warehouse plan. I also see that some of these reports are perfectly fine as CloudWatch reports run from the AWS web console...

[vrajmohan]

Decided to go with the separate queries after all (rationale - would have increased code duplication with separate reports). Will also be bringing in the LOA ACR query into this report.

[Sgtpluck]

should nil be []? (also on line 162)

[vrajmohan]

It is nil so that Array#compact can remove it. I don't know if this is idiomatic Ruby. I miss my list comprehensions.

[Sgtpluck]

oh i see, i misread what was happening here. i think it would be easier/more readable to use keep_if than collect.

issuers_with_invalid_signatures = data
  .keep_if { |slice| slice['invalid_signature_count'].to_i > 0 }
  .uniq

[vrajmohan]

That is still not enough. I would still need to do collect or map. It would have to be:

issuers_with_invalid_signatures = data
  .keep_if { |slice| slice['invalid_signature_count'].to_i > 0 }
  .collect { |slice| slice['issuer'] }
  .uniq

Would you prefer that?

[zachmargolis]

personally I like select and map but otherwise same same

issuers_with_invalid_signatures = data
  .select { |slice| slice['invalid_signature_count'].to_i > 0 }
  .map { |slice| slice['issuer'] }
  .uniq

[vrajmohan]

Strange, Rubocop wants "Place the . on the previous line, together with the method call receiver."

[Sgtpluck]

oh yeah, i don't like that particular rubocop option, but it's what we have standardized

[Sgtpluck]

I think this is fine, and if it works that's great. i have a couple suggestions about moving some of the data transformation into the data blocks so the rest of the class can ingest the data as it needs it, but i know this has been sitting for awhile so we it's not blocking (can always move things around later)

[Sgtpluck]

should we do some data transformation here, since we are only using the protocol_data method in sum_protocol_data_by? i think we could use this method to create a hash that looks like

{
  saml: $SAML_COUNT,
  oidc: $OIDC_COUNT
}

and reduce the number of methods/abstraction to get the data we need (i believe the other reporting files have examples of stuff like this, i think the inject method with a ternary is a bit hard to reason about)

[Sgtpluck]

similar to my suggestion about the protocol_data, i wonder if this makes more sense in the saml_signature_data block. if not, this is very similar to saml_issuers_with_invalid_signatures, and i wonder if that should be a method?

[Sgtpluck]

@vrajmohan this works for me!