Add ftp support to NoCloud #4834

holmanb · 2024-02-01T01:24:14Z

Proposed Commit Message

feat: Add support for FTP and FTP over TLS

Use Python's ftplib to connect to and read instance configurations from a
remove server. Like the current http support, nocloud's configuration URI
is defined via kernel command line or dmi product serial.

Notes on FTP and FTP over TLS
=============================

FTP is insecure, use at your own risk.

The FTP over TLS implementation follows library best practices[1][2].
From the ssl documentation regarding create_default_context():

> It will load the system’s trusted CA certificates, enable certificate
> validation and hostname checking, and try to choose reasonably secure
> protocol and cipher settings.

Test Changes
============

tests/i/util.py
---------------

- new helper function verify_clean_boot() to verify cloud-init state
- acquire override_kernel_cmdline() for reuse in other tests
- acquire restart_cloud_init() for reuse in other tests

tests/i/datasources/test_nocloud.py
-----------------------------------

- new test class for FTP/FTPS uses pyftpdlib for server, mkcert for test certs
- 2 test additions for FTP
- 2 test additions for FTPS

Implements via ftplib
=====================

RFC 959 - File Transfer Protocol
RFC 2640 - FTP Internationalization (UTF-8)
RFC 4217 - FTP over TLS

[1] https://docs.python.org/3/library/ftplib.html#ftp-tls-objects
[2] https://docs.python.org/3/library/ssl.html#ssl-security

Additional Context

Test Steps

PLATFORM="lxd_vm" CLOUD_INIT_SOURCE="./cloud-init_all.deb" python -m
pytest -vv --log-cli-level=INFO --durations 10 tests/integration_tests/datasources/test_nocloud.py::TestFTP

Merge type

Squash merge using "Proposed Commit Message"
Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

cloudinit/sources/DataSourceNoCloud.py

TheRealFalcon · 2024-02-01T22:08:46Z

cloudinit/util.py

@@ -2968,8 +2968,6 @@ def is_x86(uname_arch=None):


 def message_from_string(string):
-    if sys.version_info[:2] < (2, 7):


TheRealFalcon · 2024-02-08T15:50:15Z

@holmanb this still WIP?

holmanb · 2024-02-13T20:46:47Z

@holmanb this still WIP?

I'm working on an integration test for it currently, but I think it is ready for review.

TheRealFalcon

Besides the test issues, I wanted to provide my other comments here now

cloudinit/url_helper.py

TheRealFalcon · 2024-02-15T20:49:51Z

cloudinit/url_helper.py

+            return FtpResponse(url_parts.path, contents=buffer)
+        except ftplib.all_errors as e:
+            if "ftps" == url_parts.scheme:
+                raise UrlError(


This seems like an overly targeted exception for a large block of code in the try block. E.g., if we fail on the ftp_tls.retrbinary(f"RETR {url_parts.path}", callback=buffer.write) line, that wouldn't be because of the error being raised here, correct?

Can just the connect line be in the try block while letting all other exceptions escape?

Similar comment for the try after this one

Can just the connect line be in the try block while letting all other exceptions escape?

I'm hesitant to do that in the first try/except block, but I'm fine with doing it in the second.

If prot_p() throws an exception and we're using ftp:// (which permits unencrypted ftp) I think we should try to recover so we can attempt unencrypted.

@TheRealFalcon I think I've done what you asked, but I'm not certain I fully understood. Let me know.

cloudinit/url_helper.py

holmanb · 2024-02-24T03:00:37Z

@TheRealFalcon I think I've addressed your comments. I've added integration tests and updated the commit message.

Ready for re-review.

setharnold

I added some nitpicks, feel free to disregard if you disagree strongly enough. Thanks

setharnold · 2024-02-24T03:15:07Z

tests/integration_tests/datasources/test_nocloud.py

+                #
+                # instructions from https://github.com/FiloSottile/mkcert
+                assert client.execute(
+                    "git clone https://github.com/FiloSottile/mkcert && "


This changes where this can be run, and how safe it is to run this test. Could we package a fixed cert.pem and key.pem (set to expire in a hundred years or so?) to skip this download, compile, and execute?

Could we package a fixed cert.pem and key.pem (set to expire in a hundred years or so?) to skip this download, compile, and execute?

Given that this will be dropped as soon as we are no longer releasing to focal, I'm not sure that it is worth it. Also, I could use help with cert generation for that.

The challenging bit wasn't actually getting a cert.pem and key.pem, it was building a root cert and getting it into the CA store so that the client would trust it - self signed certs won't work for that. I'm not very well versed in the openssl commandline, but if someone is able to generate a test root CA, cert, and key I'd be happy to use that rather than using mkcert.

"no longer releasing to focal" may be a very long way away.

The security team has some rough-and-ready python for testing OpenSSL that might be suitable for stealing:

https://git.launchpad.net/qa-regression-testing/tree/scripts/test-openssl.py#n238

tests/integration_tests/datasources/test_nocloud.py

setharnold · 2024-02-24T03:25:57Z

tests/integration_tests/util.py

+    required_warnings_found = set()
+    required_errors_found = set()
+
+    for current_error in status["errors"]:


These loops are more complex than I'd like. They might work perfectly well, but it isn't obvious to me that they do. I would find set operations easier, eg:

missing_errors = require_errors.subtract(status["errors"]) surplus_errors = status["errors"].subtract(require_errors).subtract(ignore_errors)

(or whatever would actually run.)

I agree that this would be easier logic to follow, however set operations require exact matches rather than allowing substring matches (what the code currently does), or regex matches (what we probably want in the future). I haven't yet thought of an elegant way to accomplish this using sets.

TheRealFalcon

Generally LGTM...just mostly test requests inline.

TheRealFalcon · 2024-02-27T19:10:41Z

cloudinit/url_helper.py

-                code = NOT_FOUND
-            raise UrlError(cause=e, code=code, headers=None, url=url) from e
-        return FileResponse(file_path, contents=contents)
+    parsed = urlparse(url)


It is possible for this function to fail and raise an error. I can't think of any possible way that would happen on input that wouldn't also fail on one of the later functions, but have you considered all of the possible failure cases? I don't think the code needs to change, but just an ask to double-check any possible exceptions and ensure we won't newly fail on them.

Addressed with exception handling

TheRealFalcon · 2024-02-27T19:16:59Z

tests/integration_tests/util.py

+        return f"\n{header}:\n\t- " + "\n\t- ".join(items)
+
+
+def verify_clean_boot(


verify_clean_log has gotten pretty crusty, and this is a much nicer direction to replace it.

That said, verify_clean_log was never intended to be a collection of problems that might appear for this particular test setup. Rather, it's supposed to be a collection of problems that can appear for any test based on the platform.

E.g., every test on Azure has to work around No lease found. Every test that runs on Oracle has to work around Stderr: RTNETLINK answers: File exists.

It's obviously grown into something grosser than that, but the behavior is still required.

This happens to work for your test because you've limited your test to LXD_VMs, but if we want a general purpose utility function, we need both behaviors. At the very least I think the new function should call verify_clean_log for now. Then we can either later clean up verify_clean_log or refactor the needed pieces into this one.

At the very least I think the new function should call verify_clean_log for now.

I think that at the moment, verify_clean_log just won't work unless you pass matching expected failures. This is the result of the "Found unexpected errors/warnings" code paths - it just won't work unless you've passed everything that you expect to see in the ignored_warnings and ignored_errors keys. I don't think that calling verify_clean_log() inside of verify_clean_boot() will fix that - in fact we can't because we require failures in some of our tests, but I'm happy to call that manually as well in the integration tests that we expect a clean log in.

One idea that I envision for this helper is a matrix of platforms and series that allows us to specify exactly when an expected warning message that will be seen. This would be more precise than the current "ignore this pattern everywhere" paradigm that verify_clean_log() uses. That said, I'm not sure that I want to include that refactor in this PR, which has already grown sizeable.

tests/integration_tests/util.py

tests/integration_tests/datasources/test_nocloud.py

holmanb · 2024-03-19T01:34:53Z

@TheRealFalcon I believe that I've addressed all of your comments.

I have one more review item left to address, which is the request to include a manually generated cert rather than installing mkcert from source for focal. I won't be able to work on that request until Friday at the earliest, so if you'd like to re-review the non-test code, I don't have any more planned changes there.

TheRealFalcon

Nit: still some c variables in function signatures, but otherwise LGTM

tests/integration_tests/util.py

github-actions · 2024-04-05T00:20:29Z

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)

Make ftps:// only support ftp over TLS Make ftp:// only support unencrypted ftp Add integration tests that assert the following: ftp:// against an ftp-only server succeeds at retrieving configs (doesn't use TLS) ftp:// against an ftps-only server fails (doesn't use TLS) ftps:// against an ftps-only server succeeds at retrieving configs (uses TLS) ftps:// against an ftp-only server succeeds (cannot use TLS)

holmanb · 2024-04-15T03:06:22Z

rebased and force pushed - addressed remaining comment from @TheRealFalcon and merging

holmanb added the wip Work in progress, do not land label Feb 1, 2024

dermotbradley reviewed Feb 1, 2024

View reviewed changes

cloudinit/sources/DataSourceNoCloud.py Outdated Show resolved Hide resolved

TheRealFalcon reviewed Feb 1, 2024

View reviewed changes

TheRealFalcon self-assigned this Feb 13, 2024

holmanb added the 24.1 label Feb 13, 2024

TheRealFalcon reviewed Feb 15, 2024

View reviewed changes

TheRealFalcon reviewed Feb 16, 2024

View reviewed changes

cloudinit/url_helper.py Outdated Show resolved Hide resolved

holmanb force-pushed the holmanb/nocloud-ftp branch 4 times, most recently from abdc4bd to ec6ee22 Compare February 24, 2024 02:38

holmanb requested a review from TheRealFalcon February 24, 2024 02:58

holmanb removed the wip Work in progress, do not land label Feb 24, 2024

holmanb changed the title ~~[WIP]: Add ftp support to NoCloud~~ Add ftp support to NoCloud Feb 24, 2024

holmanb added the security label Feb 24, 2024

setharnold reviewed Feb 24, 2024

View reviewed changes

TheRealFalcon requested changes Feb 27, 2024

View reviewed changes

holmanb removed the 24.1 label Mar 12, 2024

holmanb force-pushed the holmanb/nocloud-ftp branch 2 times, most recently from 3b7dc27 to 170b061 Compare March 15, 2024 23:46

holmanb requested a review from TheRealFalcon March 19, 2024 01:31

TheRealFalcon approved these changes Mar 21, 2024

View reviewed changes

tests/integration_tests/util.py Outdated Show resolved Hide resolved

tests/integration_tests/util.py Outdated Show resolved Hide resolved

github-actions bot added the stale-pr Pull request is stale; will be auto-closed soon label Apr 5, 2024

holmanb removed the stale-pr Pull request is stale; will be auto-closed soon label Apr 12, 2024

chore: Remove Python 2 code, close BytesIO with context manager

d478042

holmanb added 20 commits April 14, 2024 15:28

feat(NoCloud): Add support for seeding from ftp

9ccdf87

comments, linters, new dependencies

bee991d

add support for custom ports for testing

3c87eca

integration-tests

d76026c

wip tests

223569e

cloudinit/url_helper.py

29b607c

deps

5b2a8c9

fmt

f7d2d7c

typing ftw

2f7aeb5

comments

d2319ed

wip

e0d1b99

fix assertion

1f55f99

fix logs

5eae499

wip

d8da402

raise UrlError rather than ValueError from urlparse

14d70e3

add verify_clean_log() calls

dd3668d

fmt

5efc909

black

b05fdb5

comments

3871f2b

holmanb force-pushed the holmanb/nocloud-ftp branch from 4adec64 to 3871f2b Compare April 14, 2024 21:31

holmanb merged commit b9aff94 into canonical:main Apr 15, 2024
28 checks passed

holmanb mentioned this pull request Jun 4, 2024

feat: Warn user of unexpected run mode #5209

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ftp support to NoCloud #4834

Add ftp support to NoCloud #4834

holmanb commented Feb 1, 2024 •

edited

Loading

TheRealFalcon Feb 1, 2024

TheRealFalcon commented Feb 8, 2024

holmanb commented Feb 13, 2024

TheRealFalcon left a comment

TheRealFalcon Feb 15, 2024

holmanb Feb 22, 2024

holmanb Feb 24, 2024

holmanb commented Feb 24, 2024 •

edited

Loading

setharnold left a comment

setharnold Feb 24, 2024

holmanb Feb 27, 2024

setharnold Feb 28, 2024

setharnold Feb 24, 2024

holmanb Mar 19, 2024

TheRealFalcon left a comment

TheRealFalcon Feb 27, 2024 •

edited

Loading

holmanb Mar 19, 2024

TheRealFalcon Feb 27, 2024

holmanb Mar 19, 2024

holmanb commented Mar 19, 2024

TheRealFalcon left a comment

github-actions bot commented Apr 5, 2024

holmanb commented Apr 15, 2024

		@@ -2968,8 +2968,6 @@ def is_x86(uname_arch=None):


		def message_from_string(string):
		if sys.version_info[:2] < (2, 7):

		return f"\n{header}:\n\t- " + "\n\t- ".join(items)


		def verify_clean_boot(

Add ftp support to NoCloud #4834

Add ftp support to NoCloud #4834

Conversation

holmanb commented Feb 1, 2024 • edited Loading

Proposed Commit Message

Additional Context

Test Steps

Merge type

Choose a reason for hiding this comment

TheRealFalcon commented Feb 8, 2024

holmanb commented Feb 13, 2024

TheRealFalcon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holmanb commented Feb 24, 2024 • edited Loading

setharnold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealFalcon left a comment

Choose a reason for hiding this comment

TheRealFalcon Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holmanb commented Mar 19, 2024

TheRealFalcon left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 5, 2024

holmanb commented Apr 15, 2024

holmanb commented Feb 1, 2024 •

edited

Loading

holmanb commented Feb 24, 2024 •

edited

Loading

TheRealFalcon Feb 27, 2024 •

edited

Loading