Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ResourceWarning in signing API #549

Open
lukpueh opened this issue Mar 14, 2023 · 12 comments
Open

ResourceWarning in signing API #549

lukpueh opened this issue Mar 14, 2023 · 12 comments
Labels
good first issue Good for newcomers qa quality assurance

Comments

@lukpueh
Copy link

lukpueh commented Mar 14, 2023

Description

Just noticed some resource warnings while unittesting. Here's a reproducer for bash on macOS:

$ python -Wdefault <<EOF
> import io
> from sigstore.oidc import Issuer
> from sigstore.sign import Signer
> issuer = Issuer.production()
> token = issuer.identity_token()
> signer = Signer.production()
> result = signer.sign(io.BytesIO(b"DATA"), token)
> EOF
Waiting for browser interaction...
<stdin>:5: ResourceWarning: unclosed <socket.socket fd=8, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0, laddr=('127.0.0.1', 59846)>
ResourceWarning: Enable tracemalloc to get the object allocation traceback
sys:1: ResourceWarning: unclosed <ssl.SSLSocket fd=9, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0, laddr=('192.168.0.154', 59853), raddr=('142.251.36.208', 443)>

Platform details:

$ sw_vers
ProductName:    macOS
ProductVersion: 12.6.3
$ python --version
Python 3.10.9
$ pip freeze | grep sigstore
sigstore==1.1.1
sigstore-protobuf-specs==0.1.0
@lukpueh lukpueh added the enhancement New feature or request label Mar 14, 2023
@woodruffw
Copy link
Member

Thanks @lukpueh!

This looks like it's in our OAuth flow.

@woodruffw woodruffw assigned tnytown and jleightcap and unassigned tnytown Mar 14, 2023
@di
Copy link
Member

di commented Mar 14, 2023

Waiting for browser interaction...

I'm curious what you're expecting to happen here? Are you planning to run this test in an environment with ambient OIDC credentials? Otherwise calling issuer.identity_token() will start a local server and attempt to open a browser to complete the OAuth flow.

@lukpueh
Copy link
Author

lukpueh commented Mar 14, 2023

Waiting for browser interaction...

I'm curious what you're expecting to happen here? Are you planning to run this test in an environment with ambient OIDC credentials? Otherwise calling issuer.identity_token() will start a local server and attempt to open a browser to complete the OAuth flow.

Oh, this snippet is just a minimal local reproducer for your convenience. My test indeed uses ambient credentials:
https://github.com/lukpueh/securesystemslib/blob/sigstore-signer-uri/tests/check_sigstore_signer.py

The related GitHub Action log also shows the resource warning:
https://github.com/lukpueh/securesystemslib/actions/runs/4417013232/jobs/7742109165

@di
Copy link
Member

di commented Mar 14, 2023

Got it, from that run it's just:

sys:1: ResourceWarning: unclosed <ssl.SSLSocket fd=4, family=2, type=1, proto=6, laddr=('10.1.1.77', 52104), raddr=('172.217.14.240', 443)>

Given that 172.217.14.240 looks like a GCS bucket, my guess is that something in our TUF flow is not closing its socket.

@lukpueh
Copy link
Author

lukpueh commented Mar 14, 2023

Got it, from that run it's just:

sys:1: ResourceWarning: unclosed <ssl.SSLSocket fd=4, family=2, type=1, proto=6, laddr=('10.1.1.77', 52104), raddr=('172.217.14.240', 443)>

Yes, that's the one I saw originally. And when I tried to reproduce locally, I got another one somewhere in issuer = Issuer.production() or token = issuer.identity_token().

@di
Copy link
Member

di commented Mar 14, 2023

I can't reproduce this. Can you call:

import warnings
warnings.filterwarnings("error")

at the top of your test case so we can get a stacktrace from these warnings instead?

@woodruffw
Copy link
Member

The full pip list might also be helpful, in case it's a particular version of TUF causing issues 🙂

@lukpueh
Copy link
Author

lukpueh commented Mar 14, 2023

EDIT: I updated the list to only contain sigstore + dependencies from a regular pip install. I still get the same warnings.

The full pip list might also be helpful, in case it's a particular version of TUF causing issues 🙂

$ pip freeze
appdirs==1.4.4
betterproto==2.0.0b5
certifi==2022.12.7
cffi==1.15.1
charset-normalizer==3.1.0
cryptography==39.0.2
grpclib==0.4.3
h2==4.1.0
hpack==4.0.0
hyperframe==6.0.1
idna==3.4
importlib-resources==5.12.0
multidict==6.0.4
pycparser==2.21
pydantic==1.10.6
PyJWT==2.6.0
pyOpenSSL==23.0.0
python-dateutil==2.8.2
requests==2.28.2
securesystemslib==0.27.0
sigstore==1.1.1
sigstore-protobuf-specs==0.1.0
six==1.16.0
tuf==2.1.0
typing_extensions==4.5.0
urllib3==1.26.15

@lukpueh
Copy link
Author

lukpueh commented Mar 14, 2023

at the top of your test case so we can get a stacktrace from these warnings instead?

lukpueh/securesystemslib@eb63de9
https://github.com/lukpueh/securesystemslib/actions/runs/4417766950/jobs/7743931502

This doesn't seem to show the stacktrace of the warnings when run with unittest.

@lukpueh
Copy link
Author

lukpueh commented Mar 14, 2023

Here's what happens when I run my snippet from above with filterwarnings("error")...

python -Wdefault <<EOF
> import warnings
> warnings.filterwarnings("error")
> import io
> from sigstore.oidc import Issuer
> from sigstore.sign import Signer
> issuer = Issuer.production()
> token = issuer.identity_token()
> signer = Signer.production()
> result = signer.sign(io.BytesIO(b"DATA"), token)
> EOF
Waiting for browser interaction...
Exception ignored in: <socket.socket fd=8, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0, laddr=('127.0.0.1', 61857)>
Traceback (most recent call last):
  File "<stdin>", line 7, in <module>
ResourceWarning: unclosed <socket.socket fd=8, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0, laddr=('127.0.0.1', 61857)>
Exception ignored in: <ssl.SSLSocket fd=9, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0, laddr=('192.168.0.154', 61865), raddr=('142.251.36.208', 443)>
ResourceWarning: unclosed <ssl.SSLSocket fd=9, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0, laddr=('192.168.0.154', 61865), raddr=('142.251.36.208', 443)>

line 7 would be token = issuer.identity_token()

@woodruffw woodruffw added good first issue Good for newcomers qa quality assurance and removed enhancement New feature or request labels Apr 28, 2023
@hojoungjang
Copy link

I looked into this a bit, and looks like the error is originating from the requests library's session object in our oidc.Issuer object when it is left unclosed.

self.session = requests.Session()

Here is a related issue thread.

My understanding is this is just a warning on unclosed resource. Based on this thread's history, I was not too sure what was the impact of this warning on @lukpueh 's side.

If we really want to get rid of this warning, we can perhaps add destructor in oidc.Issuer or refactor the class to support with statement.

@woodruffw
Copy link
Member

Yeah, closing it with a destructor seems appropriate. However, I'm considering this very low priority, since my more immediate plan is to replace requests entirely with just urllib3: #1040.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers qa quality assurance
Projects
None yet
Development

No branches or pull requests

6 participants