public icon indicating copy to clipboard operation
public copied to clipboard

629- Update authentication for VRRP

Open sbarguil opened this issue 2 years ago • 9 comments

sbarguil avatar May 03 '22 16:05 sbarguil

Compatibility Report for commit 7414aa275cb87e20199bc3d0bce655a5469ac98d: ⛔ yanglint@SO 1.10.17

OpenConfigBot avatar May 03 '22 16:05 OpenConfigBot

#629

sbarguil avatar May 03 '22 18:05 sbarguil

@sbarguil I think that this was the PR that you mentioned where there needed to be some discussion of what we did in IS-IS, is that right? Could you remind me of the question :-) Thanks.

robshakir avatar May 06 '22 16:05 robshakir

@sbarguil I think that this was the PR that you mentioned where there needed to be some discussion of what we did in IS-IS, is that right? Could you remind me of the question :-) Thanks.

Hi @Rob, yes the main question was the following:

In ISIS we have two groupings for authentication:

  • isis-authentication-type-config:
    • This grouping defines the ISIS authentication type.
    • The leaf 'AUTH_TYPE' uses the key-chain definition. I.e: SIMPLE_KEY, KEYCHAIN...etc
  • isis-simple-key-authentication-config: This grouping defines ISIS simple authentication config
    • This grouping imports the definition from the ISIS_TYPEs.
    • The leaf 'AUTH_MODE' uses the values: I.e: TEXT, MD5...etc

IMHO this two groupings partially duplicate information in leaves that imports values from two different YANGs; For example, the option TEXT defined in oc-isis-types can be equall to the option SIMPLE_KEY defined in oc-keychain-types.

In the VRRP authentication I have created a single container importing the auth information from the Key-Chain Model:

grouping ip-vrrp-simple-key-authentication-config {
    description
      "This grouping defines VRRP simple authentication config.";

    leaf auth-mode {
      type identityref {
        base oc-keychain-types:AUTH_TYPE;
      }
      description
        "The type of authentication used in the applicable IS-IS PDUs.
        This leaf along with the sibling leaf 'auth-password' can be used
        to configure the simple key authentication.";
    }

    leaf auth-password {
      type oc-types:routing-password;
      description
        "The authentication key used in the applicable IS-IS PDUs. The key in the
        packet may be encrypted according to the configured authentication type.";
    }
  }

Trying to replicate the isis-simple-key-authentication-config gruping defined in ISIS.

sbarguil avatar May 10 '22 12:05 sbarguil

@sbarguil can you please provide the following: reference to at least 2 vendor implementations of this feature (urls to documentation is great)

For convenience for reviewers, can you provide the following tree output (for example from pyang -f tree) before and after so we can easily visualize the difference?

Thanks for your help making it faster/easier to review!

dplore avatar Jun 03 '22 02:06 dplore

/gcbrun to re run checks

dplore avatar Jun 03 '22 02:06 dplore

@sbarguil can you please provide the following: reference to at least 2 vendor implementations of this feature (urls to documentation is great)

For convenience for reviewers, can you provide the following tree output (for example from pyang -f tree) before and after so we can easily visualize the difference?

Thanks for your help making it faster/easier to review!

Hi @dplore, here are the references:

  • Cisco configuration guide for VRRP: Link
  • Juniper VRRP config guide (only for ipv4): Link
  • Nokia VRRP authentication (only for ipv4):Link

sbarguil avatar Jun 07 '22 15:06 sbarguil

To address the repetition of the key-auth, perhaps a new keychain/openconfig-keychain-common.yang could be introduced.

Also, I think a leaf for auth-type is missing. Ref using isis example: https://github.com/openconfig/public/blob/b34db05e8cf2efe69df3762d4bbd80665e1f9e79/release/models/isis/openconfig-isis.yang#L292-L327

dplore avatar Jun 07 '22 20:06 dplore

w.r.t the fact that IS-IS has both SIMPLE_KEY within a keychain and TEXT, this was intentional since we noted that it wasn't possible to map some implementations to the keychain model (and there tended to be support for both). As implementations catch up to supporting keychain, we could choose to deprecate the configuration directly within the IS-IS model.

If there are implementations that (in the VRRP context) map to just keychain, then this seems reasonable not to have the same approach that we have in IS-IS and just use keychain.

robshakir avatar Jun 08 '22 22:06 robshakir