SqlServerDsc icon indicating copy to clipboard operation
SqlServerDsc copied to clipboard

SqlLogin: Should support rename parameter

Open quillypowers opened this issue 5 years ago • 10 comments

Feature request: A rename parameter would be helpful. Example would be renaming the default SA account prior to disabling per most security guidelines.

quillypowers avatar Jul 30 '20 12:07 quillypowers

The problem is that the login is a Key property, in this case the configuration would use 'sa' as that key property and that the Ensure property defaults to 'Present'. Meaning the login 'sa' should exist with the current implementation. For the SqlLogin resource to handle rename that logic must be circumvented.

Should SqlLogin instance with key Name = 'oldaccount' just do the rename (e.g 'renameaccount') and another SqlLogin instance with key Name = 'renameaccount' should enforce the other properties? Having the same instance both rename and enforce properties could be a bit counter intuitive? 🤔

We could potentially do a rename using the following configuration. Unless the Name is absent a rename is done so the Name becomes absent.

        SqlLogin 'Remove_WindowsUser'
        {
            Ensure       = 'Absent'
            Name         = 'CONTOSO\WindowsUser'
            NewName      = 'CONTOSO\RenameUser'
            LoginType    = 'WindowsUser'
            ServerName   = 'TestServer.company.local'
            InstanceName = 'DSC'
        }

johlju avatar Jul 31 '20 07:07 johlju

Happily review a PR for this change.

johlju avatar Jul 31 '20 07:07 johlju

Your change looks like a good approach, thanks for the design/thought/time.

quillypowers avatar Jul 31 '20 16:07 quillypowers

The problem with renames on keys is that if you've multiple renames on keys (even via another field as suggested) across multiple resources, you can end up in scenarios where you've duplicated keys/names across the set of resources (and their former names) and the ordering of resources within the configuration becomes more important and requires additional care to maintain/manage.

It also makes them more dependent on the exact, current state, rather than being able to start at any state and transition into a desired state because the state at the point in time has to be more specific in order for the resources to transition correctly. In some cases, it's not avoidable (e.g. if the DSC key has to be an alternate key to the actual key, such as configuration being applied against an API where the actual key is generated by the API and the DSC resource has to use another key such as "Name").

Also, just having this functionality, solely for use of the sa login/account seems quite bespoke to make on this resource when resources to add and remove resources (e.g. via ensure Present and Absent, respectively would suffice). Is there any reason why this can't be achieved with the following "Add" and "Remove" configuration(s) so a new login is added before the old 'sa' one is removed?


        SqlLogin 'Replacement_sa'
        {
            Ensure       = 'Present'
            Name         = 'SomeNewAdminLogin'
            LoginType    = 'SqlLogin'
            ServerName   = 'TestServer.company.local'
            InstanceName = 'DSC'
        }

        SqlLogin 'Remove_sa'
        {
            Ensure       = 'Absent'
            Name         = 'sa'
            LoginType    = 'SqlLogin'
            ServerName   = 'TestServer.company.local'
            InstanceName = 'DSC'

            DependsOn = @(
              '[SqlLogin]Replacement_sa'
            )
        }



I can't remember if the sa login can easily be removed via this method?

Other options for rename are via the SqlScript resource (examples here)

SphenicPaul avatar Dec 14 '20 18:12 SphenicPaul

You can't remove the SA account


From: Paul J. Wilcox [email protected] Sent: Monday, December 14, 2020 6:36 PM To: dsccommunity/SqlServerDsc [email protected] Cc: quillypowers [email protected]; Author [email protected] Subject: Re: [dsccommunity/SqlServerDsc] SqlLogin: Should support rename parameter (#1606)

The problem with renames on keys is that if you've multiple renames on keys (even via another field as suggested) across multiple resources, you can end up in scenarios where you've duplicated keys/names across the set of resources (and their former names) and the ordering of resources within the configuration becomes more important and requires additional care to maintain/manage.

It also makes them more dependent on the exact, current state, rather than being able to start at any state and transition into a desired state because the state at the point in time has to be more specific in order for the resources to transition correctly. In some cases, it's not avoidable (e.g. if the DSC key has to be an alternate key to the actual key, such as configuration being applied against an API where the actual key is generated by the API and the DSC resource has to use another key such as "Name").

Also, just having this functionality, solely for use of the sa login/account seems quite bespoke to make on this resource when resources to add and remove resources (e.g. via ensure Present and Absent, respectively would suffice). Is there any reason why this can't be achieved with the following "Add" and "Remove" configuration(s) so a new login is added before the old 'sa' one is removed?

    SqlLogin 'Replacement_sa'
    {
        Ensure       = 'Present'
        Name         = 'SomeNewAdminLogin'
        LoginType    = 'WindowsUser'
        ServerName   = 'TestServer.company.local'
        InstanceName = 'DSC'
    }

    SqlLogin 'Remove_sa'
    {
        Ensure       = 'Absent'
        Name         = 'sa'
        LoginType    = 'WindowsUser'
        ServerName   = 'TestServer.company.local'
        InstanceName = 'DSC'

        DependsOn = @(
          '[SqlLogin]Replacement_sa'
        )
    }

I can't remember if the sa login can easily be removed via this method?

Other options for rename are via the SqlScript resource (examples herehttps://github.com/dsccommunity/SqlServerDsc/tree/883013cfdb1ef951d121324934c31feb5a008260/source/Examples/Resources/SqlScript)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/dsccommunity/SqlServerDsc/issues/1606#issuecomment-744630708, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AIYSHVI6XPBQJNHVYCZIG3DSUZLLVANCNFSM4PNTPRVA.

quillypowers avatar Dec 22 '20 21:12 quillypowers

Maybe we can add a composite resource that uses resource SqlScript to do this instead of adding this functionality to the SqlLogin? The composite resource should be hard-coded to rename the 'sa' account to a optional name to avoid the ping-pong behavior that @SphenicPaul mentioned.

johlju avatar Dec 24 '20 10:12 johlju

See composite resource documentation here: https://docs.microsoft.com/en-us/powershell/scripting/dsc/resources/authoringresourcecomposite?view=powershell-5.1

johlju avatar Dec 24 '20 10:12 johlju

Also, another option might be to leave the ‘sa’ login but set a random, super-strong password and remove it (the ‘sa’ login) from all server roles and database user mappings and/or disable the login (once you’ve added other ‘sysadmin’, server role, members.

These actions wouldn’t remove the login but would remove the permissions/access (e.g. sysadmin) it has by default.

SphenicPaul avatar Dec 24 '20 11:12 SphenicPaul

The 'sa' can't be removed from all server roles, like the server role sysadmin, and not 'public' either as it is owner of that server role. But agree that before disabling (and renamed) it should be set to a super-strong password and remove it from any roles it can be removed from.

The composite resource could also set a user defined password. But could only remove it from roles and mappings that are known for a default installation (if any can be removed).

johlju avatar Dec 24 '20 12:12 johlju

@johlju I tool the SqlScriptQuery approach as follows and something similar could become a composite.

SqlDisableSa should use SUSER_NAME(0x01) so the two steps work in either order. I wasn't sure of entirely correct way to do this with SqlScriptQuery so that's left as an exercise for the reader 😅 I used DependsOn in the meantime. Any improvements welcome! 🙏

SqlScriptQuery 'SqlDisableSa' {
    InstanceName     = $Node.SqlInstanceName
    DisableVariables = $true
    GetQuery         = 'SELECT is_disabled FROM sys.server_principals WHERE sid = 0x01'
    SetQuery         = 'ALTER LOGIN sa DISABLE'
    TestQuery        = @'
IF ((SELECT COUNT(name) FROM sys.server_principals WHERE sid = 0x01 AND is_disabled = 0) = 1)
    THROW 50120, 'The sa login has not been disabled', 1
ELSE
    PRINT 'The sa login has been disabled'
'@
    DependsOn        = '[SqlSetup]SqlServer'
}

SqlScriptQuery 'SqlRenameSa' {
    InstanceName     = $Node.SqlInstanceName
    DisableVariables = $true
    GetQuery         = 'SELECT name FROM sys.server_principals WHERE sid = 0x01'
    SetQuery         = "ALTER LOGIN sa WITH NAME = $($Node.SqlRenamedSaLoginName)"
    TestQuery        = @'
IF ((SELECT COUNT(name) FROM sys.server_principals WHERE sid = 0x01 AND name = 'sa') = 1)
    THROW 50121, 'The sa login has not been renamed', 1
ELSE
    PRINT 'The sa login has been renamed'
'@
    DependsOn        = @(
        '[SqlSetup]SqlServer'
        '[SqlScriptQuery]SqlDisableSa'
    )
}

whereisaaron avatar Oct 21 '22 12:10 whereisaaron