Fix packaging/security issues in extracted GitLab module + GitLab user provisioning in dtaas-cli - #1750
Conversation
|
@8ohamed thanks for the PR. One correction about the file layout. The new code moved yesterday has to go into |
@prasadtalasila The issue was that if the dtaas-gitlab is outside the dtaas-services then publishing and packaging dtaas-services broke, that's why I moved the dtaas-gitlab inside dtaas-services, and later dtaas-cli would import/depend on dtaas-services to use dtaas-gitlab. Should I move dtaas-gitlab out again, and then add in the build flow of dtaas-services a step that copies the dtaas-gitlab src code before packaging and publishing, and gitignoring it? |
Please add the step of copying and publishing. Don't make the two CLIs ddependent |
|
@prasadtalasila I moved it back out, and added the build step "python -m dtaas_services.pkg.build" same style as the copying templates in dtaas-cli. |
@8ohamed, it might be better to add a command (or hook) into poetry build step. |
@prasadtalasila we tried that with the dtaas-cli and it introduced the problem with platform specific pip package. |
|
@prasadtalasila shoyld I also refactor the gitlab module at the top-level cli in this PR? |
makes sense. please do. |
8ohamed
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/distributed-demo #1750 +/- ##
============================================================
+ Coverage 97.00% 98.18% +1.18%
============================================================
Files 185 42 -143
Lines 6239 2595 -3644
Branches 1037 0 -1037
============================================================
- Hits 6052 2548 -3504
+ Misses 184 47 -137
+ Partials 3 0 -3
... and 204 files with indirect coverage changes
🚀 New features to boost your workflow:
|
|
Hi @prasadtalasila I added GitLab user provisioning in dtaas-cli dtaas user add can now create a GitLab account + PAT Token for each new user |
|
@8ohamed please see this review from Claude Review — GitLab module packaging fixes and CLI user provisioningVerdict: request changes. Three blockers, seven security findings, five lower items. ScopeThe change set has grown well beyond its stated scope. It now spans five commits:
The last two are a new feature — end-to-end GitLab account and Personal Access Verification performedCloned the head revision and built both consumers from source.
Both vendoring scripts work: BlockersB1 —
|
| Revision | Tests collected |
|---|---|
| base | 83 |
after 98383fc |
93 |
after 1e68641 |
71 |
Restoring the base versions of those four files and running them unmodified
against the head source tree:
4 failed, 31 passed
The four failures are all the same trivial cause — stage_users_for_add now
returns (added, passwords) instead of added, so
test_add_single_user, test_add_users_with_file,
test_add_users_file_import_error, and test_stage_single_user_registers
assert against a tuple. Each needs a one-line update, not deletion.
The other 31 were untouched by this change and were deleted anyway. Among them:
test_add_single_user/test_add_users_with_file— the happy paths of the
very command this change modifiestest_delete_user_success,test_lifecycle_command_success,
test_stop_rejects_starting_user,test_user_pause_all_targets_registry,
test_user_status_single_user- five
resolve_usernames/reject_starting_usersguards - six
get_users/get_starting_usersconfig guards - four
utilsguards includingtest_import_yaml_file_not_found
Line coverage stays at 98% because these paths are still reached indirectly,
so coverage tooling reports no regression. The behavioural assertions are
nonetheless gone.
Fix: restore all 33, updating the four tuple assertions.
B3 — lib/gitlab_common has no CI, and now two published wheels depend on it
No workflow path filter matches lib/**:
| Workflow | Filter |
|---|---|
lib-ms.yml |
servers/lib/** |
python-cli.yml |
cli/** |
platform-services-cli.yml |
deploy/services/cli/** |
lint-scripts.yml |
**.py, **.sh, **.yml (lint only, no tests) |
Both src/pkg/build.py and dtaas_services/pkg/build.py copy
lib/gitlab_common/gitlab_common into their package tree at build time. A
change confined to lib/ therefore alters two published wheels while
triggering zero test runs — worse than the single-consumer situation this was
raised against previously. This is also the direct cause of the repeated 0%
new-code coverage report.
Fix: add lib/** to the paths: filters of both python-cli.yml and
platform-services-cli.yml, or give lib/gitlab_common its own workflow that
runs its tests and then runs both consumers' suites.
Security findings
S1 — CA-bundle support added to the library is discarded at the CLI call site
lib/gitlab_common/client.py now types the parameter correctly:
def get_gitlab_client(url, private_token, *, ssl_verify: bool | str = True)with a docstring explaining that a CA bundle path is preferable to disabling
verification. cli/src/pkg/config.py then does:
return bool(section.get("ssl_verify", True)), NoneConfirmed by parsing a config containing ssl_verify="/etc/ssl/certs/corp-ca.pem":
the value reaches Config intact as a string, and bool() turns it into True.
Consequence for the primary deployment target — a self-hosted GitLab behind an
internal CA: the operator configures the bundle path, it is silently ignored,
verification falls back to the system trust store and fails, and the only
remaining lever in the config surface is ssl_verify=false. That path carries
an administrator PAT and users' initial passwords, unverified.
Nothing warns when verification is disabled, either. The library deliberately
does not suppress InsecureRequestWarning (leaving it to the application), and
the CLI does not surface it.
Fix: in get_gitlab_ssl_verify, pass a non-empty string through unchanged
and coerce only genuine booleans; emit a warning when the resolved value is
False.
S2 — write_secret_file exposes the token at 0644 before chmod
tmp.write_text(content, encoding=encoding)
os.chmod(tmp, 0o600)
os.replace(tmp, path)Instrumenting the write to stat the temp file at the moment content lands:
tmp file mode at moment of write: 0o644
final mode: 0o600
The window is short but the content is a set of GitLab PATs, and DTaaS hosts
are multi-user by design — that is the product. The temp name is also
predictable (gitlab_user_tokens.json.tmp) and opened without O_EXCL, so an
existing symlink at that path is followed.
Fix:
fd = os.open(tmp, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600)
with os.fdopen(fd, "w", encoding=encoding) as handle:
handle.write(content)
os.replace(tmp, path)This closes both issues at once: the file never exists with any other mode, and
O_EXCL refuses a pre-existing symlink.
S3 — "already exists" is reported as success, discarding a documented warning
gitlab_common.create_user carries an explicit warning in its docstring: on
HTTP 409 the account belongs to whoever registered it, the supplied password is
not applied, and callers should not issue a token against an account they did
not create. CreateOutcome.ALREADY_EXISTS exists precisely to make that
visible.
The provisioner then flattens it:
if result.outcome is CreateOutcome.ALREADY_EXISTS:
return ProvisionResult(username, True, "GitLab account already exists.")ok=True, and the caller only echoes a message when ok is false. So on a
GitLab instance that permits self-registration, a pre-claimed username results
in: a provisioned workspace container, a success message, no token, and a
DTaaS account whose GitLab identity is controlled by a third party. The
operator sees nothing unusual.
Declining to issue a PAT is correct. Reporting it as an unremarkable success is
not.
Fix: surface ALREADY_EXISTS distinctly — echo it unconditionally, and word
it as a warning that the account was not created by this run and its
credentials are unknown.
S4 — --password is exposed via shell history and the process list
The interactive prompt covers exactly one case: a single-user add, provisioning
enabled, --password omitted. Every other path takes the password from the
command line or from a plaintext CSV column. The code comment in cmd_user.py
names the risk ("where it would be visible in shell history and the process
list") while the option is offered and documented without that caveat.
Fix: document the exposure alongside the --password row in the options
table, and recommend the CSV or an environment variable for non-interactive
use — with a note to chmod the CSV.
S5 — provisioning failures never reach the exit code
_provision_gitlab_users echoes each failure and returns None; add_users
returns None. Every user's GitLab provisioning can fail — unreachable
instance, expired admin token, rejected passwords — and the command still exits
0 with "Users added successfully".
Treating GitLab failure as non-fatal for container provisioning is a
reasonable design choice. Reporting overall success is not, and it makes the
feature unusable from a script.
Fix: track whether any user was targeted and failed, and return a non-zero
exit (or a distinct message) while still leaving container provisioning intact.
S6 — no retry path after a partial failure
If the account is created but PAT issuance fails, the user is already in the
registry. A re-run of user add skips registered usernames, so added excludes
them, so the password map is empty for them, so provisioning is never retried.
The account exists with a password the operator supplied and no token; recovery
is manual.
Fix: either issue the PAT under the same failure-handling umbrella as a
retryable step, or provide an explicit re-provision path for a
registry-resident user.
S7 — [gitlab] is not validated
config_validate.py has no entries for the new section. provision=true with
a malformed api_url or an empty pat surfaces only when provisioning runs,
after containers have been created.
Lower
-
Direct-URL metadata guard is unsound on failure. The check is
unzip -p dist/*.whl '*.dist-info/METADATA' | grep -E ...without
set -o pipefail. Ifunzipfails, the pipeline's status is grep's, grep
finds nothing, theifis false, and the step prints
"OK: no direct-URL dependencies". It also inspects only the wheel; the sdist
is built in the same step and never checked. Addpipefail, assert the
METADATA extraction produced output, and check the sdist'sPKG-INFO. -
Neither vendored copy carries provenance.
src/gitlab_common/and
dtaas_services/gitlab_common/are byte copies with no version or source
commit recorded. With two consumers, silent divergence between an installed
wheel andlib/is now twice as likely. Stamp a__source_version__during
vendoring. -
No
lib/gitlab_common/poetry.lock. Still outstanding. -
Token file location is undocumented. The README says issued tokens are
saved togitlab_user_tokens.jsonbut not that the path is relative to the
working directory, that it is mode 0600, that it accumulates across runs, or
that it must be treated as a credential store. (The working-directory
relativity is consistent withdtaas.users.registry.json, so it is a
documentation gap rather than a behavioural inconsistency.) -
Description is stale. It describes
dtaas_services/gitlab_core/, which
no longer exists, and does not mention the CLI provisioning feature that
accounts for most of the diff.
Confirmed resolved
The earlier packaging and library findings are all closed, verified rather than
read:
- No
@ file://entry in built wheel metadata; both wheel and sdist carry the
vendored source; a clean-venv install imports successfully. - Global warning-state mutation removed from the library and returned to the
application entry point. ssl_verifyaccepts a CA bundle path at the library layer (see S1 for the
CLI-side regression).PatOptionswith least-privilege defaults (read_repository,
write_repository) and UTC expiry;apiscope now opted into explicitly by
the one caller that needs it.CreateOutcomeenum replacing the nullable-id encoding, with a docstring
warning about the already-exists case.py.typedpresent; README signatures corrected; documentation updated at
three levels.- Passwords are structurally kept out of the registry:
_passwords_to_addand
read_csv_passwordsare deliberately independent of the registry-details
path, and the registry write never sees a password field.
|
@prasadtalasila I have resolved the issues found by claude, and I fixed the qlty issues |
|
@8ohamed please see the revised review from Claude Review — GitLab module packaging and CLI user provisioning (round 3)Verdict: request changes. One blocker carried over unaddressed, one new What changed since the last reviewThree commits added: Verification performedRebuilt both consumers from the new head and re-ran every measurement from the
Blocker (carried over, unaddressed)B1 — the 33 deleted tests are still goneThe previous round measured 33 pre-existing tests deleted by the "improves test All 33. None was renamed, parametrized, or relocated into Still missing, among others: Two of them are now more valuable than when they were deleted, not less: Four of the 33 need a one-line update for the New findingH1 — repeated
|
please check the functionality of both |
|
|
@prasadtalasila I fixed the issues, and tested the package on the server. |
|
@8ohamed please keep the old tests as well. Are both packages tested on the server? |
Yes I have tested it for both |
2 participants
Fix packaging/security issues in extracted GitLab module + GitLab user provisioning in dtaas-cli
Type of Change
Description
Fixes review findings on the earlier GitLab extraction (#1748). The shared GitLab code now lives in
lib/gitlab_common/as a standalone package and is copied intodtaas-servicesat build time, rather than published separately or embedded directly.lib/gitlab_common/standalone package (ownpyproject.toml, own tests), single source of truth.dtaas_services/pkg/build.pycopieslib/gitlab_common/gitlab_common/intodtaas_services/gitlab_common/run-build-script: true) before both tests and packaging; locally it must be run manually documented inDEVELOPER.mdandREADME.md.gitlab_commonno longer mutates global warning state SSL-warning suppression moved back to the application entry point (_api.py).ssl_verifynow accepts a CA-bundle path, not just bool.PatOptions), defaulting to least-privilege repository scopes;dtaas_servicesopts intoapiscope explicitly.create_userreturns aCreateOutcomeenum (CREATED/ALREADY_EXISTS/FAILED) instead of encoding "already exists" as a nullable id.py.typed, and a CI step that fails the build if wheel metadata contains a direct-URL dependency.Uses
gitlab_commonin the DTaaS CLI, adding the capability of the gitlab_common. The CLI had no GitLab code before this (only OAuth URLs indtaas.toml).dtaas user addcan now create each new user's GitLab account and a Personal Access Token, gated behind[gitlab].provisionindtaas.toml(defaultfalseno behaviour change unless explicitly enabled).cli/src/pkg/gitlab/(client.py+provisioner.py) is built entirely ongitlab_common's client and user/PAT primitives no GitLab logic is reimplemented in the CLI.--password(prompted with hidden input if omitted, for a single-user add) or a newpasswordcolumn inusers.csv. It is used once to create the account and is never written todtaas.users.registry.json,.dtaas.state.json, or logs.gitlab_user_tokens.json(mode0600).