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

Map piv/cac to the Federal Bridge CA if using valid policy OIDs (LG-3604) #193

Merged
merged 12 commits into from
Jan 8, 2021

Conversation

mitchellhenke
Copy link
Contributor

Creates a new service that will attempt to verify a cert is issued by a trusted authority and has valid policy OIDs. The service is only permitted to download bundles from specified CA issuers via configuration. Missing hosts will be logged so we can consider adding them in the future. These hosts will also have to be coordinated with the outbound proxy configuration.

Policy OIDs are only checked for the downloaded certs. If a cert is stored, we continue to not check policy to avoid breaking existing functionality.

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM!

app/models/certificate.rb Outdated Show resolved Hide resolved
app/models/certificate.rb Outdated Show resolved Hide resolved
app/services/issuing_ca_service.rb Outdated Show resolved Hide resolved
Comment on lines 9 to 18
return nil unless cert.aia && cert.aia['CA Issuers'].is_a?(Array)
ca_issuers = cert.aia['CA Issuers'].map do |issuer|
issuer = issuer.to_s
next unless issuer.starts_with?('URI')
issuer = issuer.gsub(/^URI:/, '')
uri = URI.parse(issuer)
next unless uri.scheme == 'http'
next unless allowed_host?(uri.host)
uri
end.compact
Copy link
Contributor

Choose a reason for hiding this comment

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

a few things:

  • Some ideas for avoiding unless
  • Ideas for avoiding reassigning issuer param:
  • possibly simpler control flow with fewer early returns?

this is subjective, just an idea

Suggested change
return nil unless cert.aia && cert.aia['CA Issuers'].is_a?(Array)
ca_issuers = cert.aia['CA Issuers'].map do |issuer|
issuer = issuer.to_s
next unless issuer.starts_with?('URI')
issuer = issuer.gsub(/^URI:/, '')
uri = URI.parse(issuer)
next unless uri.scheme == 'http'
next unless allowed_host?(uri.host)
uri
end.compact
ca_issuers = Array(cert.aia && cert.aia['CA Issuers']).map do |issuer|
if issuer.to_s.starts_with?('URI:')
uri = URI.parse(issuer.to_s.gsub(/^URI:/, ''))
uri if uri.scheme == 'http' && allowed_host?(uri.host)
end
end.compact

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Acknowledging the subjectivity for myself as well on this. I'm not committed to what we have at the moment, but I have difficulty parsing nested calls and implicit returns while reading. It's lengthier, but the intention was a series of clear steps for filtering and transformation.

I was considering something like filter_map, but it would have to be filter -> map -> filter which felt harder to follow.

Copy link
Contributor

Choose a reason for hiding this comment

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

there's flat_map which will remove nil values automatically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think flat_map will only pull values out of arrays

irb(main):003:0> ['a', nil, ['a']].flat_map { |x| x }
=> ["a", nil, "a"]

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah right, well probably fine to leave as-is for a bit then

config/application.yml.example Outdated Show resolved Hide resolved
app/services/issuing_ca_service.rb Show resolved Hide resolved
mitchellhenke and others added 3 commits January 7, 2021 13:01
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/federal-bridge-and-policy-oids branch from 304c0ff to 9983fa3 Compare January 7, 2021 20:06
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/federal-bridge-and-policy-oids branch from 9983fa3 to 32134d0 Compare January 7, 2021 20:10
Copy link
Member

@jmhooper jmhooper left a comment

Choose a reason for hiding this comment

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

🚢

@mitchellhenke mitchellhenke merged commit c653cbe into master Jan 8, 2021
@mitchellhenke mitchellhenke deleted the mitchellhenke/federal-bridge-and-policy-oids branch January 8, 2021 15:14
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