Skip to content

Sync changes from WinRb/WinRM upstream - #36

Merged
tpowell-progress merged 6 commits into
chef:mainfrom
tas50:sync/winrb-upstream
Sep 18, 2026
Merged

tpowell-progress merged 6 commits into
chef:mainfrom
tas50:sync/winrb-upstream

Conversation

@tas50

@tas50 tas50 commented Sep 14, 2026

Copy link
Copy Markdown

Summary

Brings across the changes made in WinRb/WinRM since the two projects diverged at v2.3.9 (2024-08-02), so the forks stay reconcilable.

Rebased onto current main (9710771). Upstream has 16 commits since that point. Each was reviewed individually rather than merged wholesale — a straight merge would have dragged in ~135 files of rename and quote-style churn, since this fork moved lib/winrm/ to lib/chef-winrm/ and reformatted to cookstyle --chefstyle. The five commits here are hand-ported and follow this repo's conventions.

What's included

Replace the logging gem with the standard library Logger (upstream WinRb#358, WinRb#360)

Removes a runtime dependency. The gem was used in exactly three places: parsing WINRM_LOG, building Connection#logger, and building a second unrelated logger inside HttpTransport. WinRM.default_log_level now handles WINRM_LOG, warning and falling back to :warn on an unrecognized value.

Connection#logger returns WinRM::CompatLogger, a stdlib Logger subclass that still accepts add_appenders — the one logging-specific call consumers actually used. It warns once per instance and does nothing, so existing code keeps running rather than raising NoMethodError on upgrade.

This also fixes a latent bug: the transport built its own logger, so a logger assigned to Connection#logger never reached it. The transport now receives the connection's logger.

Close orphaned shells via at_exit when finalizers can't spawn threads (upstream WinRb#354)

Ruby 3.1 forbids allocating a thread inside a finalizer, which this repo worked around by making the finalizer a no-op. That stopped the ThreadError but leaked the remote shell: it survived on the WinRM server until its idle timeout expired, occupying one of the server's limited shell slots meanwhile.

The finalizer now attempts the thread as before and, when the interpreter refuses with can't alloc thread, defers the close to at_exit. Unrecognized ThreadErrors still propagate, and a failure inside the deferred close warns rather than raising, since an exception from at_exit would obscure the program's real exit status. Adds coverage for all four paths; the no-op version had none.

Exclude the broken rubyntlm 0.6.7

Not part of the upstream sync, but CI surfaced it here and it blocks everything.

rubyntlm 0.6.7, released 2026-09-14, extracted the session key derivation methods into Net::NTLM::Client::SessionCrypto but left CLIENT_TO_SERVER_SEALING and the other three key constants behind in Net::NTLM::Client::Session. Ruby resolves a constant referenced in a method body through the lexical scope where the method was written and that scope's ancestors, not through the ancestors of the class that later includes the module, so Session is never searched. All four derivations raise NameError.

The negotiate transport reaches them via seal_message/unseal_message, so with 0.6.7 installed every NTLM and Negotiate connection fails. Negotiate is the default transport, so this breaks essentially all usage on a fresh install. 0.6.5 and 0.6.6 are unaffected; the constraint excludes only 0.6.7 so a fixed 0.6.8 is picked up automatically.

Nothing in the unit suite touched that path, so it only failed in the integration suite, which needs a live Windows host. Added a contract spec covering all four derivations — it fails in seconds against 0.6.7 and passes against 0.6.5 and 0.6.6.

Fix the dead AppVeyor link in the certificate auth instructions (upstream WinRb#364) — the AppVeyor removal itself landed separately in #33 while this PR was open, so rebasing reduced this commit to its residual: the README pointed at WinrmAppveyor.psm1 on the WinRb/WinRM master branch as an openssl example, and that link is doubly dead now that the branch is main and the file is gone from both repos. It names openssl directly instead.

Rename changelog.md to CHANGELOG.md — GitHub and RubyGems don't surface the lowercase spelling.

Run the unit workflow on pushes to main — its push trigger still named master, a branch this repo doesn't have, so unit tests ran on PRs but never on merge. Noticed while reviewing the upstream CI commits.

Deliberately not included

Fork-specific choices left untouched: chef-gyoku, Ruby >= 3.1, cookstyle over rubocop, and the pinned rexml >= 3.4.2 for CVE fixes.

Testing

  • bundle exec rake spec — 256 examples, 0 failures, 2 pending (was 238 on main; +18 new tests covering the finalizer paths, CompatLogger, default_log_level, and the rubyntlm key derivations)
  • bundle exec cookstyle --chefstyle -c .rubocop.yml — no offenses on any changed file (the two Style/ModuleMemberExistenceCheck offenses it reports are pre-existing on main, in files this PR does not touch)
  • Verified at runtime that the logging gem is no longer loaded, the transport shares the connection's logger, and both the valid and invalid WINRM_LOG paths behave correctly

@tas50
tas50 force-pushed the sync/winrb-upstream branch from bbbcfd0 to 773b9ea Compare September 15, 2026 15:41
The README pointed at WinrmAppveyor.psm1 on the WinRb/WinRM master branch
for an example of minting a client-auth certificate with openssl. That
link is doubly dead: the branch is now main, and the file itself is gone
from both repos, deleted here in chef#33 and upstream in WinRb#364.
Name openssl directly instead.

Signed-off-by: Tim Smith <tim@mondoo.com>
GitHub, RubyGems and most tooling look for the uppercase spelling and
will not surface a lowercase changelog.md. Rename it and update the two
references in the copilot instructions.

Renamed via an intermediate filename so the change is recorded correctly
on case-insensitive filesystems.

Ported from WinRb/WinRM 2827ae1.

Signed-off-by: Tim Smith <tim@mondoo.com>
The unit workflow's push trigger still named master, a branch this
repository does not have. Unit tests therefore ran on pull requests but
never on merge, so main had no post-merge verification. The lint
workflow already triggers on main; this brings unit into line.

Ported from WinRb/WinRM 915d688.

Signed-off-by: Tim Smith <tim@mondoo.com>
Ruby 3.1 forbids allocating a thread inside a finalizer, so the original
`Thread.new { close_shell }` finalizer raised ThreadError during garbage
collection. That was worked around by making the finalizer a no-op, which
stopped the crash but leaked the remote shell: an unclosed shell survived
on the WinRM server until its idle timeout expired, consuming one of the
server's limited shell slots in the meantime.

Attempt the thread as before, and when the interpreter refuses with
"can't alloc thread", register an at_exit block that closes the shell at
process exit instead. Unrecognized ThreadErrors still propagate, and a
failure inside the deferred close warns rather than raising, since an
exception from at_exit would obscure the program's real exit status.

Adds coverage for all four paths, which the no-op version had none of.

Ported from WinRb#354.

Signed-off-by: Tim Smith <tim@mondoo.com>
The logging gem was the only reason this library carried a logging
dependency at all, and it was used for three things: reading a level out
of WINRM_LOG, building a logger for Connection, and building a second,
unrelated logger inside HttpTransport. Ruby's standard library covers all
three, so drop the dependency.

WinRM.default_log_level now parses WINRM_LOG, warning and falling back to
:warn on an unrecognized value rather than relying on the logging gem to
raise ArgumentError. The transport takes its logger from the connection
via options[:logger] instead of constructing its own, so a logger a
caller assigns to Connection#logger now reaches the transport too, which
it previously did not.

Connection#logger returns WinRM::CompatLogger, a stdlib Logger subclass
that still accepts add_appenders. It is the one logging-specific call
consumers actually used; it now emits a deprecation warning once per
instance and does nothing, so code written against the old API keeps
running instead of raising NoMethodError on upgrade.

Ported from WinRb#358 and WinRb#360.

Signed-off-by: Tim Smith <tim@mondoo.com>
rubyntlm 0.6.7 extracted the session key derivation methods into
Net::NTLM::Client::SessionCrypto but left CLIENT_TO_SERVER_SEALING and the
other three key constants behind in Net::NTLM::Client::Session. Ruby
resolves a constant referenced in a method body through the lexical scope
where the method was written and that scope's ancestors, not through the
ancestors of the class that later includes the module, so Session is never
searched. Every call to client_seal_key, server_seal_key, client_sign_key
and server_sign_key raises NameError.

The negotiate transport reaches those methods through seal_message and
unseal_message, so with 0.6.7 installed every NTLM and Negotiate
connection fails. Negotiate is the default transport, so this breaks
essentially all usage. 0.6.5 and 0.6.6 are unaffected, and the dependency
now excludes only 0.6.7 so a fixed 0.6.8 is picked up automatically.

Nothing in the unit suite touched that code path, so the breakage showed
up only in the integration suite, which needs a live Windows host. Add a
contract spec covering all four derivations; it fails in seconds against
0.6.7 and passes against 0.6.5 and 0.6.6.

Signed-off-by: Tim Smith <tim@mondoo.com>
@tas50
tas50 force-pushed the sync/winrb-upstream branch from ab89585 to 4784551 Compare September 15, 2026 18:53
@tpowell-progress
tpowell-progress merged commit b41fe52 into chef:main Sep 18, 2026
39 checks passed
@tas50
tas50 deleted the sync/winrb-upstream branch September 18, 2026 15:27
tpowell-progress pushed a commit that referenced this pull request Sep 18, 2026
…ng together (#37)

The unit workflow has been red on main since #36. Two merged pull
requests are individually fine and incompatible together:

* #35 added `disable_monkey_patching!` and `verify_partial_doubles` to
  the spec helper, so a top level group must be `RSpec.describe` and a
  partial double may only stub methods the real object defines.
* #36 ported spec files from WinRb/WinRM that predate those settings.

Neither pull request could see the conflict, because each was tested
against a base that did not yet contain the other, and the unit workflow
did not run on pushes to main until #36 itself turned that on.

Two distinct failures result.

`compat_logger_spec.rb`, `rubyntlm_contract_spec.rb` and the `.finalize`
group appended to `shells/base_spec.rb` open with a bare top level
`describe`, which is no longer defined on main, so all three files fail
to load with NoMethodError and take the whole run down with them. Use
`RSpec.describe`, as the other forty spec files already do.

With the files loading again, the `.finalize` examples fail: they stub
`close_shell` on `WinRM::Shells::Base`, which does not implement it.
That is not a mistake in the stub but the point of the class. Base
leaves `close_shell` to its subclasses and calls it from the finalizer
proc, where it dispatches on the concrete class. `verify_partial_doubles`
is reporting this accurately.

The existing examples in the file drive that path through `DummyShell`,
but it overrides `finalize` outright and so cannot exercise the real
implementation. Add a second subclass that inherits `finalize` and
supplies only `close_shell`, and point the group at it.

No library code changes. The suite is 254 examples, 0 failures, verified
across several seeds, and `rake style` is clean.

Signed-off-by: Tim Smith <tim@mondoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants