Skip to content

[yang]: AAA login pattern#9805

Merged
liuh-80 merged 3 commits intosonic-net:masterfrom
ganglyu:aaa_login
Mar 7, 2022
Merged

[yang]: AAA login pattern#9805
liuh-80 merged 3 commits intosonic-net:masterfrom
ganglyu:aaa_login

Conversation

@ganglyu
Copy link
Contributor

@ganglyu ganglyu commented Jan 20, 2022

Signed-off-by: Gang Lv [email protected]

Why I did it

end2end test is blocked by Yang model for AAA login pattern.

How I did it

Add pattern to AAA yang models.

How to verify it

Run UT for sonc-yang-models.

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106

Description for the changelog

Fix #9713

A picture of a cute animal (not mandatory but encouraged)

@ganglyu ganglyu added the YANG YANG model related changes label Jan 20, 2022
Fix typo
9c49558 
Signed-off-by: Gang Lv [email protected]
@ganglyu
Copy link
Contributor Author

ganglyu commented Jan 20, 2022

/azpw run Azure.sonic-buildimage

@mssonicbld
Copy link
Collaborator

mssonicbld commented Jan 20, 2022

/AzurePipelines run Azure.sonic-buildimage

@azure-pipelines
Copy link

azure-pipelines bot commented Jan 20, 2022

Azure Pipelines successfully started running 1 pipeline(s).

wen587
wen587 previously approved these changes Jan 20, 2022
View reviewed changes
Copy link
Contributor

@wen587 wen587 left a comment

Choose a reason for hiding this comment

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

Verified locally

@qiluo-msft
Copy link
Collaborator

qiluo-msft commented Jan 21, 2022

@liuh-80 @renukamanavalan Could you help review?

qiluo-msft
qiluo-msft previously approved these changes Jan 21, 2022
View reviewed changes
Copy link
Collaborator

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

LGTM. Please wait a while for more eyes.

@wen587
Copy link
Contributor

wen587 commented Jan 21, 2022

Found an issue here:
Each AAA sub-config doesn't share the same config.
authorization: [tacacs+ | local | '"tacacs+ local"']
authentication: [ {radius, tacacs+, local} | default ]
accounting: [disable | tacacs+ | local | '"tacacs+ local"']

admin@vlab-01:~/tacacs$ sudo config aaa authorization -h
Usage: config aaa authorization [OPTIONS] [[tacacs+|local|tacacs+ local]]...

  Switch AAA authorization [tacacs+ | local | '"tacacs+ local"']               <===========

Options:
  -h, -?, --help  Show this message and exit.
admin@vlab-01:~/tacacs$ sudo config aaa authentication login -h
Usage: config aaa authentication login [OPTIONS]
                                       [[radius|tacacs+|local|default]]...         

  Switch login authentication [ {radius, tacacs+, local} | default ]         <===========

Options:
  -?, -h, --help  Show this message and exit.
admin@vlab-01:~/tacacs$ sudo config aaa accounting -h
Usage: config aaa accounting [OPTIONS] [[disable|tacacs+|local|tacacs+ 
                             local]]...

  Switch AAA accounting [disable | tacacs+ | local | '"tacacs+ local"']    <==========

Options:
  -h, -?, --help  Show this message and exit.

@wen587 wen587 dismissed their stale review January 21, 2022 09:31

New issue found.

renukamanavalan
renukamanavalan reviewed Jan 21, 2022
View reviewed changes
src/sonic-yang-models/yang-models/sonic-system-aaa.yang Outdated
type string;
description "AAA authentication/authorization/accounting methods - local/tacacs+/disable";
type string {
pattern 'radius|tacacs\+|local|default' {
Copy link
Contributor

@renukamanavalan renukamanavalan Jan 21, 2022

Choose a reason for hiding this comment

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

Do we accommodate the value "default" in SONiC auth operations?

Copy link
Contributor

@liuh-80 liuh-80 Mar 4, 2022

Choose a reason for hiding this comment

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

We do accommodate "default", the default will set authentication "login" parameter to "local" and remove all other authentication settings.

@qiluo-msft qiluo-msft dismissed their stale review January 22, 2022 02:10

Share the same concern with @renukamanavalan

dgsudharsan
dgsudharsan requested changes Feb 15, 2022
View reviewed changes
src/sonic-yang-models/yang-models/sonic-system-aaa.yang Outdated
type string;
description "AAA authentication/authorization/accounting methods - local/tacacs+/disable";
type string {
pattern 'radius|tacacs\+|local|default' {
Copy link
Collaborator

@dgsudharsan dgsudharsan Feb 15, 2022

Choose a reason for hiding this comment

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

Can you please add config into https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-yang-models/tests/files/sample_config_db.json ?

Copy link
Collaborator

@dgsudharsan dgsudharsan Feb 15, 2022

Choose a reason for hiding this comment

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

Please also add configuration schema here https://github.com/Azure/sonic-swss/blob/53c630b82d429db892bf288dd9f323c5a8370cd5/doc/Configuration.md

Copy link
Contributor

@liuh-80 liuh-80 Mar 4, 2022

Choose a reason for hiding this comment

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

AAA setting already in sample_config_db.json:

    "AAA": {
        "authentication": {
            "login": "local"
        },
        "authorization": {
            "login": "local"
        },
        "accounting": {
            "login": "local"
        }
    },

Copy link
Contributor

@liuh-80 liuh-80 Mar 4, 2022

Choose a reason for hiding this comment

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

Will update Configuration.md in another PR because it's in different repo.

Copy link
Contributor

@liuh-80 liuh-80 Mar 4, 2022

Choose a reason for hiding this comment

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

Created a new PR for update configuration.md in sonic-swss repo: sonic-net/sonic-swss#2168

@ganglyu
Copy link
Contributor Author

ganglyu commented Feb 15, 2022

Need to confirm the pattern, abandon this PR.

@ganglyu ganglyu closed this Feb 15, 2022
@ganglyu ganglyu reopened this Feb 16, 2022
@ganglyu ganglyu marked this pull request as draft February 16, 2022 02:49
liuh-80
liuh-80 reviewed Mar 3, 2022
View reviewed changes
src/sonic-yang-models/tests/yang_model_tests/tests_config/aaa.json
"sonic-system-aaa:AAA": {
"AAA_LIST": [{
"type": "authentication",
"login": "tacacs+,local",
Copy link
Contributor

@liuh-80 liuh-80 Mar 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

should keep tacacs+ here, because sonic support login with tacacs+ protocol
#Closed

Copy link
Contributor

@liuh-80 liuh-80 Mar 4, 2022

Choose a reason for hiding this comment

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

Fixed

liuh-80
liuh-80 reviewed Mar 3, 2022
View reviewed changes
src/sonic-yang-models/yang-models/sonic-system-aaa.yang Outdated
type string;
description "AAA authentication/authorization/accounting methods - local/tacacs+/disable";
type string {
pattern 'radius|tacacs\+|local|default' {
Copy link
Contributor

@liuh-80 liuh-80 Mar 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This pattern need update to support multiple login method, for example following method will try authentication with tacacs+ protocol first, if tatacs+ authentication failed, will try use local authentication:
tacacs+,local
#Closed

Copy link
Contributor

@liuh-80 liuh-80 Mar 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Suggest use this regex for pattern: ((tacacs+|local|redus|default),{0,1})+

Copy link
Contributor

@liuh-80 liuh-80 Mar 4, 2022

Choose a reason for hiding this comment

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

Fixed.

@liuh-80
Copy link
Contributor

liuh-80 commented Mar 4, 2022

After offline sync with gang, he will handover this PR to me, I will continue to finish this PR.

@ganglyu ganglyu marked this pull request as ready for review March 4, 2022 04:34
wen587
wen587 approved these changes Mar 4, 2022
View reviewed changes
Copy link
Contributor

@wen587 wen587 left a comment

Choose a reason for hiding this comment

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

Mannual test pass. LGTM

@renukamanavalan
Copy link
Contributor

renukamanavalan commented Mar 4, 2022

After offline sync with gang, he will handover this PR to me, I will continue to finish this PR.

So we wait till liuh-80 finish the PR update.

@liuh-80 liuh-80 merged commit 2ef9d65 into sonic-net:master Mar 7, 2022
@ganglyu ganglyu added the Request for 202111 Branch For PRs being requested for 202111 branch label Apr 14, 2022
judyjoseph pushed a commit that referenced this pull request Apr 18, 2022
@ganglyu @judyjoseph
[yang]: AAA login pattern (#9805)
8412606 
Signed-off-by: Gang Lv [email protected]

<!--
     Please make sure you've read and understood our contributing guidelines:
     https://github.com/Azure/SONiC/blob/gh-pages/CONTRIBUTING.md

     ** Make sure all your commits include a signature generated with `git commit -s` **

     If this is a bug fix, make sure your description includes "fixes #xxxx", or
     "closes #xxxx" or "resolves #xxxx"

     Please provide the following information:
-->

#### Why I did it
end2end test is blocked by Yang model for AAA login pattern.

#### How I did it
Add pattern to AAA yang models.

#### How to verify it
Run UT for sonc-yang-models.

#### Which release branch to backport (provide reason below if selected)

<!--
- Note we only backport fixes to a release branch, *not* features!
- Please also provide a reason for the backporting below.
- e.g.
- [x] 202006
-->

- [ ] 201811
- [ ] 201911
- [ ] 202006
- [ ] 202012
- [ ] 202106

#### Description for the changelog
<!--
Write a short (one line) summary that describes the changes in this
pull request for inclusion in the changelog:
-->
Fix #9713 

#### A picture of a cute animal (not mandatory but encouraged)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wen587 wen587 wen587 approved these changes

@qiluo-msft qiluo-msft qiluo-msft left review comments

@venkatmahalingam venkatmahalingam Awaiting requested review from venkatmahalingam venkatmahalingam is a code owner

@praveen-li praveen-li Awaiting requested review from praveen-li praveen-li is a code owner

@rathnasabapathyv rathnasabapathyv Awaiting requested review from rathnasabapathyv

@renukamanavalan renukamanavalan Awaiting requested review from renukamanavalan

@dgsudharsan dgsudharsan Awaiting requested review from dgsudharsan dgsudharsan is a code owner

@liuh-80 liuh-80 Awaiting requested review from liuh-80

Assignees

No one assigned

Labels

Included in 202111 Branch Request for 202111 Branch For PRs being requested for 202111 branch YANG YANG model related changes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[yang-models] aaa login should verify valid input

8 participants

@ganglyu @mssonicbld @qiluo-msft @wen587 @liuh-80 @renukamanavalan @dgsudharsan @judyjoseph