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

Stir/Shaken Refactor #524

Merged

Conversation

gtjoseph
Copy link
Member

@gtjoseph gtjoseph commented Jan 9, 2024

Why do we need a refactor?

The original stir/shaken implementation was started over 3 years ago
when little was understood about practical implementation. The
result was an implementation that wouldn't actually interoperate
with any other stir-shaken implementations.

There were also a number of stir-shaken features and RFC
requirements that were never implemented such as TNAuthList
certificate validation, sending Reason headers in SIP responses
when verification failed but we wished to continue the call, and
the ability to send Media Key(mky) grants in the Identity header
when the call involved DTLS.

Finally, there were some performance concerns around outgoing
calls and selection of the correct certificate and private key.
The configuration was keyed by an arbitrary name which meant that
for every outgoing call, we had to scan the entire list of
configured TNs to find the correct cert to use. With only a few
TNs configured, this wasn't an issue but if you have a thousand,
it could be.

What's changed?

  • Configuration objects have been refactored to be clearer about
    their uses and to fix issues.

    • The "general" object was renamed to "verification" since it
      contains parameters specific to the incoming verification
      process. It also never handled ca_path and crl_path
      correctly.
    • A new "attestation" object was added that controls the
      outgoing attestation process. It sets default certificates,
      keys, etc.
    • The "certificate" object was renamed to "tn" and had it's key
      change to telephone number since outgoing call attestation
      needs to look up certificates by telephone number.
    • The "profile" object had more parameters added to it that can
      override default parameters specified in the "attestation"
      and "verification" objects.
    • The "store" object was removed altogther as it was never
      implemented.
  • We now use libjwt to create outgoing Identity headers and to
    parse and validate signatures on incoming Identiy headers. Our
    previous custom implementation was much of the source of the
    interoperability issues.

  • General code cleanup and refactor.

    • Moved things to better places.
    • Separated some of the complex functions to smaller ones.
    • Using context objects rather than passing tons of parameters
      in function calls.
    • Removed some complexity and unneeded encapsuation from the
      config objects.

Resolves: #351
Resolves: #46

UserNote: Asterisk's stir-shaken feature has been refactored to
correct interoperability, RFC compliance, and performance issues.
See https://docs.asterisk.org/Deployment/STIR-SHAKEN for more
information.

UpgradeNote: The stir-shaken refactor is a breaking change but since
it's not working now we don't think it matters. The
stir_shaken.conf file has changed significantly which means that
existing ones WILL need to be changed. The stir_shaken.conf.sample
file in configs/samples/ has quite a bit more information. This is
also an ABI breaking change since some of the existing objects
needed to be changed or removed, and new ones added. Additionally,
if res_stir_shaken is enabled in menuselect, you'll need to either
have the development package for libjwt v1.15.3 installed or use
the --with-libjwt-bundled option with ./configure.

@gtjoseph
Copy link
Member Author

gtjoseph commented Jan 9, 2024

cherry-pick-to: 18
cherry-pick-to: 20
cherry-pick-to: 21

main/db.c Outdated Show resolved Hide resolved
@gtjoseph gtjoseph marked this pull request as draft January 16, 2024 20:43
@gtjoseph
Copy link
Member Author

I'm moving this back to draft because setting any of the profile config fields will wither result in a segfault or the wrong structure member being set. I'm refactoring how the profiles work to prevent the duplication of fields and to more easily prevent disagreements.

@gtjoseph
Copy link
Member Author

gtjoseph commented Feb 1, 2024

Still trying to decide what can be unit tested vs testsuite tested.

@lukeescude
Copy link
Contributor

Super excited for this!!!!!! You guys rock!!

@gtjoseph gtjoseph marked this pull request as ready for review February 6, 2024 17:17
@gtjoseph
Copy link
Member Author

gtjoseph commented Feb 6, 2024

Super excited for this!!!!!! You guys rock!!

Thanks @lukeescude! Would it be possible for you to test at all?

@lukeescude
Copy link
Contributor

Super excited for this!!!!!! You guys rock!!

Thanks @lukeescude! Would it be possible for you to test at all?

Absolutely, do you happen to know where I can find the docs for configuring the module? (Not sure if it's still following the same syntax as the original stir_shaken.conf) - I don't mind looking at the code to extrapolate docs, just need to know where.

@seanbright
Copy link
Contributor

@github-actions github-actions bot added cherry-pick-testing-in-progress Cherry-Pick tests in progress cherry-pick-checks-failed Cherry-Pick checks failed and removed cherry-pick-test Trigger dry run of cherry-picks cherry-pick-testing-in-progress Cherry-Pick tests in progress labels Feb 27, 2024
mbradeen
mbradeen previously approved these changes Feb 27, 2024
Why do we need a refactor?

The original stir/shaken implementation was started over 3 years ago
when little was understood about practical implementation.  The
result was an implementation that wouldn't actually interoperate
with any other stir-shaken implementations.

There were also a number of stir-shaken features and RFC
requirements that were never implemented such as TNAuthList
certificate validation, sending Reason headers in SIP responses
when verification failed but we wished to continue the call, and
the ability to send Media Key(mky) grants in the Identity header
when the call involved DTLS.

Finally, there were some performance concerns around outgoing
calls and selection of the correct certificate and private key.
The configuration was keyed by an arbitrary name which meant that
for every outgoing call, we had to scan the entire list of
configured TNs to find the correct cert to use.  With only a few
TNs configured, this wasn't an issue but if you have a thousand,
it could be.

What's changed?

* Configuration objects have been refactored to be clearer about
  their uses and to fix issues.
    * The "general" object was renamed to "verification" since it
      contains parameters specific to the incoming verification
      process.  It also never handled ca_path and crl_path
      correctly.
    * A new "attestation" object was added that controls the
      outgoing attestation process.  It sets default certificates,
      keys, etc.
    * The "certificate" object was renamed to "tn" and had it's key
      change to telephone number since outgoing call attestation
      needs to look up certificates by telephone number.
    * The "profile" object had more parameters added to it that can
      override default parameters specified in the "attestation"
      and "verification" objects.
    * The "store" object was removed altogther as it was never
      implemented.

* We now use libjwt to create outgoing Identity headers and to
  parse and validate signatures on incoming Identiy headers.  Our
  previous custom implementation was much of the source of the
  interoperability issues.

* General code cleanup and refactor.
    * Moved things to better places.
    * Separated some of the complex functions to smaller ones.
    * Using context objects rather than passing tons of parameters
      in function calls.
    * Removed some complexity and unneeded encapsuation from the
      config objects.

Resolves: asterisk#351
Resolves: asterisk#46

UserNote: Asterisk's stir-shaken feature has been refactored to
correct interoperability, RFC compliance, and performance issues.
See https://docs.asterisk.org/Deployment/STIR-SHAKEN for more
information.

UpgradeNote: The stir-shaken refactor is a breaking change but since
it's not working now we don't think it matters. The
stir_shaken.conf file has changed significantly which means that
existing ones WILL need to be changed.  The stir_shaken.conf.sample
file in configs/samples/ has quite a bit more information.  This is
also an ABI breaking change since some of the existing objects
needed to be changed or removed, and new ones added.  Additionally,
if res_stir_shaken is enabled in menuselect, you'll need to either
have the development package for libjwt v1.15.3 installed or use
the --with-libjwt-bundled option with ./configure.
Copy link

Successfully merged to branch master and cherry-picked to ["18","20","21"]

@gtjoseph gtjoseph deleted the master-stir-shaken-refactor branch March 20, 2024 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants