[Home]

Summary:ASTERISK-05100: [branch] show managers + show manager foobar
Reporter:Clod Patry (junky)Labels:
Date Opened:2005-09-18 00:22:59Date Closed:2008-01-15 17:11:31.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 5240.txt
( 1) 5240b.txt
( 2) junky
( 3) managerc.diff
( 4) managerh.diff
Description:That patch allow you know all infos for managers, even if managers are offline.
asterisk*CLI> show managers
username                            secret
--------                            ------
dunky3                              booo
panel                               ur_name
callback                            moooo
junky                               julie24
stef                                junkjunk
-------------------
5 manager users configured.

You can have all the info for a specific manager like:
asterisk*CLI> show manager junky
asterisk*CLI>
      username: junky
        secret: julie24
          deny: 0.0.0.0/0.0.0.0
        permit: 127.0.0.1
          read: system,call,log,verbose,agent,user
         write: system,call,log,verbose,command,agent,user
displayconnects: no
asterisk*CLI>

I also add the show managers concise:
asterisk*CLI> show managers concise
asterisk*CLI>
dunky3:booo
panel:ur_name
callback:moooo
junky:julie24
stef:junkjunk
asterisk*CLI>



****** ADDITIONAL INFORMATION ******

Also, all previous commands with show manager*
command    commands   connected
works too.

The only restriction by that, is you cannot have a manager user named command, commands, connected.


That patch is the first part for something really useful: bypass the parsing of the manager.conf on each authentication.

If you change something in the manager.conf and it will change in the memory, just after a reload manager.
Comments:By: Chuck Pearce (vulturecp) 2005-09-18 02:09:46

Confirmed to work with HEAD-9-18-05

asterisk*CLI> show managers
username                            secret
--------                            ------
admin                               ******
-------------------
1 manager users configured.
asterisk*CLI> show manager admin
asterisk*CLI>
      username: admin
        secret: ******
          deny: 0.0.0.0/0.0.0.0
        permit: 127.0.0.1/255.255.255.0
          read: system,call,log,verbose,command,agent,user
         write: system,call,log,verbose,command,agent,user
displayconnects: no

asterisk*CLI> show managers concise
asterisk*CLI>
admin:*****


By: Clod Patry (junky) 2005-09-18 02:15:05

great, can we deprecate ASTERISK-4897 , since that one only print write permission for session vs mine for all users (online, offline) and print a lot more infos about manager users.

Btw, id like to say a great thanks to anthm, for his help for that one.
By: Matt O'Gorman (mogorman) 2005-09-21 23:25:55

I just tested it for junky.  Works great no problems.

Matt
By: Clod Patry (junky) 2005-11-27 01:28:18.000-0600

Just re-tested that patch, it still applicable with latest cvs-head.
By: Tilghman Lesher (tilghman) 2005-11-28 08:56:21.000-0600

Is it really wise to print the manager secrets?  The manager protocol currently does a 2-step process in order to hide the password from being sent in the clear, but since the manager session itself is not encrypted, anyone with manager access and Command privileges could unwittingly compromise all manager passwords by sending "Command: show managers".

In a number of other places (including during module reload), we intentionally don't print passwords, to protect them.
By: Clod Patry (junky) 2005-11-28 11:15:56.000-0600

I could remove the print of secret, for both show managers and show manager foo.
What do you think about all the other stuff?
By: Tilghman Lesher (tilghman) 2005-11-28 12:00:49.000-0600

Architecturally, we have a problem, in that we've decided that all commands should follow the model of "module action parameters", such that your commands should be "manager show users" and "manager show user <foo>".

You may also want to parse the read and write permissions into bitwise fields, then prepare routines to go from the bitwise fields to text.  This would ensure that repeated fields are resolved into one and that the text names would also always be in the same order.  It also makes checking permissions a whole lot faster, as a bitwise comparison is always faster than a strcasestr().

If you're going to store manager users in a linked list, then you need to provide a method by which to re-read the manager users during a reload (and dispose of the old linked list, keeping in mind that some manager users may still be connected).

I'm also opposed to removing the current commands, as your patch currently does.  Knowing which managers are currently connected and being able to read the manager help online are both still useful.

Instead of storing the string fields of permit/deny, you should resolve them into a linked list of (struct ast_ha *)'s, using the routines in acl.c to build the list.

If you provide a public API to add manager users to the list, you must also provide a public API to remove same users.  Note that you'll need some mechanism to prevent removing connected manager users (or marking them such that the struct goes away when they disconnect, while preventing new connections).  Also, I think that ast_manager_add is a poor name choice, given that we already have ast_manager_register and ast_manager_unregister.  Perhaps a better choice would be ast_manager_user_add ?  Alternatively, we could simply not make adding a user a public API (preferring, instead, to force administrators to change manager.conf and issue a reload).
By: Clod Patry (junky) 2005-11-28 17:26:09.000-0600

Okay for manager show users and manager show user foo.
I just based my code on the show channels and show channel foo concept.

About "read and write permissions into bitwise fields", i'll be in touch with you on IRC.

Even a reload doesnt touched my code, since an online manager is still a struct "mansession". If you see a leak here, let me know please.

These functions still works, just the tab completition which is not.
see:
       if ( !strcasecmp(argv[2],"connected") )
               return handle_showmanconn(fd,argc,argv);
       else if ( !strcasecmp(argv[2],"commands") )
               return handle_showmancmds(fd,argc,argv);

I'll take a deeper look at struct ast_ha and acl.c, thanks for spot.

I've no problem changing my function's name to ast_manager_user_add(), that's just a name, like any other. What code the code in that function?
I'll create ast_manager_user_remove(), sounds good for you? I created my ast_manager_add() non-static just to make sure all managers are added via the "reload manager" in the CLI. Should i remove the prototype from the header?
Once again, even if a specific manager is online, reloading the list wont affect it, since online manager are a mansession object.

I would like to remind, this is just the base to incorporate the manager concept in *, 2nd step will be to integrate the mansession in the ast_manager_user and to disable the whole parsing of the manager.conf on EACH connect attempt.
By: Tilghman Lesher (tilghman) 2005-11-28 18:34:05.000-0600

You don't need a public API for adding users to connect your "manager reload" command to a function which reloads the users -- just set up a static function and register the CLI with a pointer to that function.  Actually, you don't need even that, because reload_manager() is already linked in to loader.c, such that "reload manager" works.  You only need to make sure that you properly deallocate the old list when init_manager() is called.
By: Clod Patry (junky) 2005-12-31 06:41:46.000-0600

Here are my modifications for 5240b.txt
Remove 2 functions from the API.
Remove the secret fields in both displays.
Rename ast_manager_add()  to ast_manager_user_add().
Change "show manager" foo to "manager show user foo".
Change "show managers" to "manager show users".
Set *amus = NULL at the beginning of init_manager().
"current commands" are now back with tabs completion.

I did not create the ast_manager_user_remove(), since ast_manager_user_add() isn't in the API anymore.

About the deny/permit, it's just for online users, no? Don't forget, manager show users is for both, online and offline users.
By: Tilghman Lesher (tilghman) 2006-03-01 16:05:08.000-0600

This patch no longer cleanly applies.
By: Olle Johansson (oej) 2006-03-04 04:17:08.000-0600

Integrating this into test-this-branch
By: Clod Patry (junky) 2006-03-04 09:55:22.000-0600

oej, just msg me on IRC, if you need any help related to this patch.
By: Olle Johansson (oej) 2006-03-06 03:42:09.000-0600

Junky: Added small fix by Luigi Rizzo (he's got a disclaimer on file). Implemented that in test-this-branch. please integrate with your patch. Recommend you open a branch for this to keep it up to date.... :-)
By: Clod Patry (junky) 2006-03-06 08:43:25.000-0600

good idea.
i will take a look at his patch.
Btw, thanks Luigi for all hard work.
By: Serge Vecher (serge-v) 2006-05-30 09:28:59

junky: any progress here?
By: Clod Patry (junky) 2006-05-30 19:03:09

not really, just waiting feedbacks from the test-this-branch.
By: Serge Vecher (serge-v) 2006-06-05 12:33:29

junky: test-this-branch is broken :( Can you please update your patch to latest trunk, I'll help you test this -- so we can hopefully get this into 1.4 beta. Thanks
By: Serge Vecher (serge-v) 2006-08-25 11:07:39

junky: any chance of an updated patch here?
By: Clod Patry (junky) 2006-08-25 17:39:57

since its post 1.4, i dont want to write it 20 times, so i will write it back when 1.4 comes out.
By: Anthony LaMantia (alamantia) 2006-09-18 15:32:11

I have uploaded files for patching this aginst trunk.
and it is included in my testing branch /team/anthonyl/5240-testing

this is something that we want to include in 1.4 still, any updates comments..etc would be quite helpfull right now.
By: Tilghman Lesher (tilghman) 2006-09-18 15:56:25

alamantia:  there's a massive memory leak in your code.  Everytime you call "reload manager" it's going to leak a memory structure of all of your managers previously loaded, because you're setting amus to NULL without deallocating the memory first.
By: Jason Parker (jparker) 2006-09-19 17:02:25

Committed to svn trunk in revision 43300.
By: Clod Patry (junky) 2006-09-19 17:20:06

yay!
if you need any help with that patch, let me know.
By: Clod Patry (junky) 2007-02-13 12:22:14.000-0600

I,ve update one production machine to SVN-branch-1.4-r51087M.
And i remarqued it still parsing the manager.conf on each connection!
The main goal is the original patch was to remove these parsing on each connections.
Is this something we would like to remove?

By: Serge Vecher (serge-v) 2007-03-07 11:54:42.000-0600

junky: since the feature patch has been committed, let's pursue the parsing problem in a separate report. Thanks.
By: Digium Subversion (svnbot) 2008-01-15 17:11:31.000-0600

Repository: asterisk
Revision: 11776

U   team/oej/test-this-branch/README.test-this-branch
U   team/oej/test-this-branch/include/asterisk/manager.h
U   team/oej/test-this-branch/manager.c

------------------------------------------------------------------------
r11776 | oej | 2008-01-15 17:11:30 -0600 (Tue, 15 Jan 2008) | 2 lines

Integrating ASTERISK-5100 - Show manager CLI commands (junky)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=11776