#9583 batch is failing on missing attribute principal in error case using server context
Closed: fixed by rcritten. Opened by twoerner.

Issue

context.principal is used for the info log in the error case of the execution of the commands. But this attribute does not seem to be set in all cases (using server context), which results in a trace in the error handling. Therefore the execution is aborted and real failure is not returned.

Steps to Reproduce

  1. Create batch command with several commands, with an command that will result in an error, for example "no modification to be done"

Actual behavior

Trace back and aborted execution

Expected behavior

No failure and full processing of all commands.

Version/Release/Distribution

$ rpm -q freeipa-server freeipa-client ipa-server ipa-client 389-ds-base pki-ca krb5-server
ipa-server-4.11.0-5.el9.x86_64
ipa-client-4.11.0-5.el9.x86_64
389-ds-base-2.4.5-3.el9.x86_64
krb5-server-1.21.1-1.el9.x86_64


Please provide logs and a reproducer json content. context.principal should be set for all HTTPS-endpoint initiated calls.

The reproducer:

batch-test.py

Current Output
Init

Add host batch-test-host-01.cos9.local

Run batch:
[{'method': 'host_mod',
'params': (['batch-test-host-01.cos9.local'], {'updatedns': True})},
{'method': 'host_mod',
'params': (['batch-test-host-01.cos9.local'], {'location': 'bar'})}]

AttributeError("'_thread._local' object has no attribute 'principal'")
name 'ret' is not defined

location: None

Expected Output
Init

Add host batch-test-host-01.cos9.local

Run batch:
[{'method': 'host_mod',
'params': (['batch-test-host-01.cos9.local'], {'updatedns': True})},
{'method': 'host_mod',
'params': (['batch-test-host-01.cos9.local'], {'location': 'bar'})}]

ipa: INFO: UNKNOWN: batch: host_mod('batch-test-host-01.cos9.local', updatedns=True): EmptyModlist
ipa: INFO: UNKNOWN: batch: ConversionError
("{'count': 2, 'results': [{'error': 'no modifications to be performed', "
"'error_code': 4202, 'error_name': 'EmptyModlist', 'error_kw': {}}, {'error': "
'"invalid \'params\': Unknown option: location", \'error_code\': 3008, '
"'error_name': 'ConversionError', 'error_kw': {'name': 'params', 'error': "
"'Unknown option: location'}}], 'messages': [{'type': 'warning', 'name': "
'\'VersionMissing\', \'message\': "API Version number was not sent, forward '
'compatibility not guaranteed. Assuming server\'s API version, 2.253", '
"'code': 13001, 'data': {'server_version': '2.253'}}]}")

location: None

This reproducer uses server context which is not expected to provide context.principal because it uses LDAPI with UID binding and is not GSSAPI authentication. We still need to fix places where context.principal is referenced to avoid those issues.

Worse batch

batch_args = [
    {
        "method": "host_mod",
        "params": ([host_fqdn],
                   {
                       "nshostlocation": "foo",
                   })
    },
    {
        "method": "host_mod",
        "params": ([host_fqdn],
                   {
                       "updatedns": True,
                   })
    },
    {
        "method": "host_mod",
        "params": ([host_fqdn],
                   {
                       "nshostlocation": "bar",
                   })
    },
]

** Current result**
Init

Add host batch-test-host-01.cos9.local

Run batch:
[{'method': 'host_mod',
'params': (['batch-test-host-01.cos9.local'], {'nshostlocation': 'foo'})},
{'method': 'host_mod',
'params': (['batch-test-host-01.cos9.local'], {'updatedns': True})},
{'method': 'host_mod',
'params': (['batch-test-host-01.cos9.local'], {'nshostlocation': 'bar'})}]

ipa: INFO: UNKNOWN: batch: host_mod('batch-test-host-01.cos9.local', nshostlocation='foo'): SUCCESS
AttributeError("'_thread._local' object has no attribute 'principal'")
name 'ret' is not defined

location: ['foo']

Expected result
Init

Add host batch-test-host-01.cos9.local

Run batch:
[{'method': 'host_mod',
'params': (['batch-test-host-01.cos9.local'], {'nshostlocation': 'foo'})},
{'method': 'host_mod',
'params': (['batch-test-host-01.cos9.local'], {'updatedns': True})},
{'method': 'host_mod',
'params': (['batch-test-host-01.cos9.local'], {'nshostlocation': 'bar'})}]

ipa: INFO: UNKNOWN: batch: host_mod('batch-test-host-01.cos9.local', nshostlocation='foo'): SUCCESS
ipa: INFO: UNKNOWN: batch: host_mod('batch-test-host-01.cos9.local', updatedns=True): EmptyModlist
ipa: INFO: UNKNOWN: batch: host_mod('batch-test-host-01.cos9.local', nshostlocation='bar'): SUCCESS
("{'count': 3, 'results': [{'result': {'krbprincipalname': "
"[ipapython.kerberos.Principal('host/batch-test-host-01.cos9.local@COS9.LOCAL')], "
"'fqdn': ['batch-test-host-01.cos9.local'], 'nshostlocation': ['foo'], "
"'krbcanonicalname': "
"[ipapython.kerberos.Principal('host/batch-test-host-01.cos9.local@COS9.LOCAL')], "
"'has_password': False, 'has_keytab': False, 'managedby_host': "
"['batch-test-host-01.cos9.local']}, 'value': "
"'batch-test-host-01.cos9.local', 'summary': 'Modified host "
'"batch-test-host-01.cos9.local"\', \'error\': None}, {\'error\': \'no '
"modifications to be performed', 'error_code': 4202, 'error_name': "
"'EmptyModlist', 'error_kw': {}}, {'result': {'krbprincipalname': "
"[ipapython.kerberos.Principal('host/batch-test-host-01.cos9.local@COS9.LOCAL')], "
"'fqdn': ['batch-test-host-01.cos9.local'], 'nshostlocation': ['bar'], "
"'krbcanonicalname': "
"[ipapython.kerberos.Principal('host/batch-test-host-01.cos9.local@COS9.LOCAL')], "
"'has_password': False, 'has_keytab': False, 'managedby_host': "
"['batch-test-host-01.cos9.local']}, 'value': "
"'batch-test-host-01.cos9.local', 'summary': 'Modified host "
'"batch-test-host-01.cos9.local"\', \'error\': None}], \'messages\': '
'[{\'type\': \'warning\', \'name\': \'VersionMissing\', \'message\': "API '
'Version number was not sent, forward compatibility not guaranteed. Assuming '
'server\'s API version, 2.253", \'code\': 13001, \'data\': '
"{'server_version': '2.253'}}]}")

location: ['bar']

@twoerner can you try https://github.com/freeipa/freeipa/pull/7335 ?

@abbra About batch, why aren't you changing line 177, like I did in PR https://github.com/freeipa/freeipa/pull/7332/ ?

Using just 'UNKNOWN' is incorrect. We have security checks there in place, they need to be done regardless whether context principal is set or not. And that means validating things before we dereference context.principal.

There is getattr(context, 'principal', 'UNKNOWN') in line 177, shouldn't this be replaced with op_account also?

There are more places where getattr(context, 'principal') is used in plugins.

Understood.

Why these comments are here and not in the pull request review, btw?

Yeah, good point.

master:

  • 295ac6385c33d28502396ffeb9e7a5297b63a005 privilege: use context.principal only when it is defined
  • 3608b2b63de736186176f8da0fad36ce3b0d57a3 batch: account for auto-binding in server context
  • 71d886f0713b2c58d8eb57f2267d59cc0be39345 config: use context.principal only when it is defined
  • ab5465639d5c083d6396551f06c450fe4d349d1b server: use context.principal only when it is defined
  • 08f1e6f2fdb19db681c0560db53a7a5fa1ce3784 trust: use context.principal only when it is defined
  • b6131b57371f6eade697125a4500c140997478c0 trust: handle stray pylint warning
  • e386e22046fec4de062116245a3cd9e79c457499 cert: use context.principal only when it is defined
  • 902c8b0bae90b04d3f1d91f0703c2a0eca4e39f1 passwd: handle LDAP auto-bind use case as well
  • c325f9c045787a4c4e18096e23cb2f84f514b28e user: handle LDAP auto-bind for whoami case
  • 6cc0a0b9a8439f97cfc688e5610c25b5e494ba0b pylint: use yield_from for trivial cases
  • 9e861693fcb79d256af6d0cfe26f27c7f7ff8e13 batch: add keeponly option

ipa-4-11:

  • 3d03ab4b9c0309418a9d90e4672e6ae4a4f0bec0 privilege: use context.principal only when it is defined
  • 83a68fb47221e173e707c8f1305a0ca417d4def7 batch: account for auto-binding in server context
  • 68c072de1a91dd629f16855ef6b0aad9e4f83949 config: use context.principal only when it is defined
  • 75c50a01b89b3170949e035006f9409c8d9c63c5 server: use context.principal only when it is defined
  • b9ccc679be38924d1928a3547b1fb883b9048d75 trust: use context.principal only when it is defined
  • 092b5814f60648c7787250a9e1593ff7472fc9e5 trust: handle stray pylint warning
  • e582634314a3bc8316534f5afb9cedd60bfe8935 cert: use context.principal only when it is defined
  • e1177a65a67e77ae12cadd1f977b467c36fa026f passwd: handle LDAP auto-bind use case as well
  • 3b07e25d7bf90cac094094f8f83cae0c9c480c27 user: handle LDAP auto-bind for whoami case
  • 5117a53585c22a8b13682be8088e4598f051ffca pylint: use yield_from for trivial cases
  • 88abda57d53c25cb9a4d306edd212f44045d6e46 batch: add keeponly option

Metadata Update from @rcritten:
- Issue close_status updated to: fixed
- Issue status updated to: Closed (was: Open)

Metadata