Possible regression in samba-tool provision

[Andrew Bartlett]

On Fri, 2023-03-24 at 10:10 -0400, John Mulligan via samba-technical
wrote:
> Hi samba team,
> 
> One of our projects consumes container images based on nightly rpm builds of 
> samba master. Over the last day or two one of our test jobs has been failing 
> and I think it is due to recent changes.  This might only affect mit krb5 based 
> builds.

Thanks John so much for testing and getting in touch.  We greatly
appreciate those who test master!

> When provisioning a domain we see a traceback like so:
> 
> INFO 2023-03-23 21:22:50,399 pid:6 /usr/lib64/python3.10/site-packages/samba/
> provision/__init__.py #2021: Fixing provision GUIDs
> ERROR(<class 'AttributeError'>): uncaught exception - 'DomainUpdate' object 
> has no attribute 'upper'
>   File "/usr/lib64/python3.10/site-packages/samba/netcmd/__init__.py", line 
> 230, in _run
>     return self.run(*args, **kwargs)
>   File "/usr/lib64/python3.10/site-packages/samba/netcmd/domain.py", line 555, 
> in run
>     result = provision(self.logger,
>   File "/usr/lib64/python3.10/site-packages/samba/provision/__init__.py", line 
> 2408, in provision
>     create_kdc_conf(paths.kdcconf, realm, domain, os.path.dirname(lp.get("log 
> file")))
>   File "/usr/lib64/python3.10/site-packages/samba/provision/kerberos.py", line 
> 43, in create_kdc_conf
>     domain = domain.upper()
> Temporarily overriding 'dsdb:schema update allowed' setting
> 
> This is followed by some other logging output but the overall command fails.
> 
> I tracked this down to change 4bba26579d124af6c0767bb98bee67357001e1e7 which 
> adds some code to `python/samba/provision/__init__.py`. Part of the diff:
> 
> > +                try:
> > +                    from samba.domain_update import DomainUpdate
> > +
> > +                    domain = DomainUpdate(samdb, fix=True)
> > +                    domain.check_updates_functional_level(adprep_level,
> > +                                                         
> > DS_DOMAIN_FUNCTION_2008, +                                                 
> >         update_revision=True) +
> > +                    samdb.transaction_commit()
> > +                except Exception as e:
> > +                    samdb.transaction_cancel()
> > +                    raise e
> 
> This block uses the variable domain that gets assigned a DomainUpdate object, 
> but the lines below:
> 
> >          if not is_heimdal_built():
> >              create_kdc_conf(paths.kdcconf, realm, domain,
> >              os.path.dirname(lp.get("log file"))) logger.info("The Kerberos            
> KDC configuration for Samba AD is "
> 
> pass domain to create_kdc_conf which expect the value in domain to be a 
> string.  Skimming the code I think this block is the last to use domain 
> variable, and the only one to use it after it gets reassigned to a 
> DomainUpdate object, so it's probably only the mit krb5 build that will hit 
> this error.
> A fix might be to just rename `domain` variable in the new block or even remove 
> it and chain the 
> `DomainUpdate(...).domain.check_updates_functional_level(...)` together.

Please propose such a patch in GitLab, if you can!

> In the mean time, we think we may be able to work around this issue by using 
> the `--adprep-level` option, but we are still investigating.
> 
> If you have any other questions, comments, or would prefer I report this to 
> bugzilla please just ask.  Thanks!

I don't think we need this, it is only in master.

> --John M.
> 
> 
> PS: As a python coder, the samdb transaction handling could be written in a 
> neater way using a context manager. ;-) ;-)

Samba's python was written by C programmers, so moves to allow more
pythonic handling would be greatly appreciated.

Andrew Bartlett

-- 
Andrew Bartlett (he/him) https://samba.org/~abartlet/
Samba Team Member (since 2001) https://samba.org
Samba Developer, Catalyst IT https://catalyst.net.nz/services/samba