From d5b5e130c10f3552f7ab91bc2b3fbc46d244bc4b Mon Sep 17 00:00:00 2001 From: jbouwh Date: Sun, 23 Apr 2023 11:46:28 +0000 Subject: [PATCH 1/5] Add cipher list option to IMAP config flow --- homeassistant/components/imap/config_flow.py | 18 +++++++++++ homeassistant/components/imap/const.py | 1 + homeassistant/components/imap/coordinator.py | 24 ++++++++++++-- homeassistant/components/imap/strings.json | 12 ++++++- tests/components/imap/test_config_flow.py | 33 ++++++++++++++++++++ tests/components/imap/test_init.py | 15 +++++++-- 6 files changed, 96 insertions(+), 7 deletions(-) diff --git a/homeassistant/components/imap/config_flow.py b/homeassistant/components/imap/config_flow.py index a6ef203283c79..7bd34b71aff0a 100644 --- a/homeassistant/components/imap/config_flow.py +++ b/homeassistant/components/imap/config_flow.py @@ -13,18 +13,33 @@ from homeassistant.core import callback from homeassistant.data_entry_flow import AbortFlow, FlowResult from homeassistant.helpers import config_validation as cv +from homeassistant.helpers.selector import ( + SelectSelector, + SelectSelectorConfig, + SelectSelectorMode, +) +from homeassistant.util.ssl import SSLCipherList from .const import ( CONF_CHARSET, CONF_FOLDER, CONF_SEARCH, CONF_SERVER, + CONF_SSL_CIPHER_LIST, DEFAULT_PORT, DOMAIN, ) from .coordinator import connect_to_server from .errors import InvalidAuth, InvalidFolder +CIPHER_SELECTOR = SelectSelector( + SelectSelectorConfig( + options=list(SSLCipherList), + mode=SelectSelectorMode.DROPDOWN, + translation_key=CONF_SSL_CIPHER_LIST, + ) +) + CONFIG_SCHEMA = vol.Schema( { vol.Required(CONF_USERNAME): str, @@ -34,6 +49,9 @@ vol.Optional(CONF_CHARSET, default="utf-8"): str, vol.Optional(CONF_FOLDER, default="INBOX"): str, vol.Optional(CONF_SEARCH, default="UnSeen UnDeleted"): str, + vol.Optional( + CONF_SSL_CIPHER_LIST, default=SSLCipherList.PYTHON_DEFAULT + ): CIPHER_SELECTOR, } ) diff --git a/homeassistant/components/imap/const.py b/homeassistant/components/imap/const.py index 080f7bf676578..a1ca586b48b1f 100644 --- a/homeassistant/components/imap/const.py +++ b/homeassistant/components/imap/const.py @@ -8,5 +8,6 @@ CONF_FOLDER: Final = "folder" CONF_SEARCH: Final = "search" CONF_CHARSET: Final = "charset" +CONF_SSL_CIPHER_LIST: Final = "ssl_cipher_list" DEFAULT_PORT: Final = 993 diff --git a/homeassistant/components/imap/coordinator.py b/homeassistant/components/imap/coordinator.py index e11cf1e0baf41..9df6c77631163 100644 --- a/homeassistant/components/imap/coordinator.py +++ b/homeassistant/components/imap/coordinator.py @@ -6,6 +6,7 @@ from datetime import datetime, timedelta import email import logging +import ssl from typing import Any from aioimaplib import AUTH, IMAP4_SSL, NONAUTH, SELECTED, AioImapException @@ -21,8 +22,16 @@ from homeassistant.core import HomeAssistant from homeassistant.exceptions import ConfigEntryAuthFailed, ConfigEntryError from homeassistant.helpers.update_coordinator import DataUpdateCoordinator, UpdateFailed - -from .const import CONF_CHARSET, CONF_FOLDER, CONF_SEARCH, CONF_SERVER, DOMAIN +from homeassistant.util.ssl import SSL_CIPHER_LISTS, SSLCipherList + +from .const import ( + CONF_CHARSET, + CONF_FOLDER, + CONF_SEARCH, + CONF_SERVER, + CONF_SSL_CIPHER_LIST, + DOMAIN, +) from .errors import InvalidAuth, InvalidFolder _LOGGER = logging.getLogger(__name__) @@ -34,8 +43,17 @@ async def connect_to_server(data: Mapping[str, Any]) -> IMAP4_SSL: """Connect to imap server and return client.""" - client = IMAP4_SSL(data[CONF_SERVER], data[CONF_PORT]) + if ( + ciphers := data.get(CONF_SSL_CIPHER_LIST, SSLCipherList.PYTHON_DEFAULT) + ) != SSLCipherList.PYTHON_DEFAULT: + context = ssl.create_default_context() + context.set_ciphers(SSL_CIPHER_LISTS[SSLCipherList(ciphers)]) + client = IMAP4_SSL(data[CONF_SERVER], data[CONF_PORT], ssl_context=context) + else: + client = IMAP4_SSL(data[CONF_SERVER], data[CONF_PORT]) + await client.wait_hello_from_server() + if client.protocol.state == NONAUTH: await client.login(data[CONF_USERNAME], data[CONF_PASSWORD]) if client.protocol.state not in {AUTH, SELECTED}: diff --git a/homeassistant/components/imap/strings.json b/homeassistant/components/imap/strings.json index d104f591c6387..bcd7994f2df01 100644 --- a/homeassistant/components/imap/strings.json +++ b/homeassistant/components/imap/strings.json @@ -9,7 +9,8 @@ "port": "[%key:common::config_flow::data::port%]", "charset": "Character set", "folder": "Folder", - "search": "IMAP search" + "search": "IMAP search", + "ssl_cipher_list": "SSL cipher list (Advanced)" } }, "reauth_confirm": { @@ -49,5 +50,14 @@ "invalid_folder": "[%key:component::imap::config::error::invalid_folder%]", "invalid_search": "[%key:component::imap::config::error::invalid_search%]" } + }, + "selector": { + "ssl_cipher_list": { + "options": { + "python_default": "Default settings", + "modern": "Modern ciphers", + "intermediate": "Intermediate ciphers" + } + } } } diff --git a/tests/components/imap/test_config_flow.py b/tests/components/imap/test_config_flow.py index 098efb4280fcc..43275818c15e9 100644 --- a/tests/components/imap/test_config_flow.py +++ b/tests/components/imap/test_config_flow.py @@ -27,6 +27,7 @@ "charset": "utf-8", "folder": "INBOX", "search": "UnSeen UnDeleted", + "ssl_cipher_list": "python_default", } MOCK_OPTIONS = { @@ -426,6 +427,7 @@ async def test_import_flow_success(hass: HomeAssistant) -> None: "charset": "utf-8", "folder": "INBOX", "search": "UnSeen UnDeleted", + "ssl_cipher_list": "python_default", } assert len(mock_setup_entry.mock_calls) == 1 @@ -455,3 +457,34 @@ async def test_import_flow_connection_error(hass: HomeAssistant) -> None: assert result["type"] == FlowResultType.ABORT assert result["reason"] == "cannot_connect" + + +@pytest.mark.parametrize("cipher_list", ["python_default", "modern", "intermediate"]) +async def test_config_flow_with_cipherlist( + hass: HomeAssistant, mock_setup_entry: AsyncMock, cipher_list: str +) -> None: + """Test with alternate cipherlist.""" + config = MOCK_CONFIG.copy() + config["ssl_cipher_list"] = cipher_list + result = await hass.config_entries.flow.async_init( + DOMAIN, context={"source": config_entries.SOURCE_USER} + ) + assert result["type"] == FlowResultType.FORM + assert result["errors"] is None + + with patch( + "homeassistant.components.imap.config_flow.connect_to_server" + ) as mock_client: + mock_client.return_value.search.return_value = ( + "OK", + [b""], + ) + result2 = await hass.config_entries.flow.async_configure( + result["flow_id"], config + ) + await hass.async_block_till_done() + + assert result2["type"] == FlowResultType.CREATE_ENTRY + assert result2["title"] == "email@email.com" + assert result2["data"] == config + assert len(mock_setup_entry.mock_calls) == 1 diff --git a/tests/components/imap/test_init.py b/tests/components/imap/test_init.py index 60efde71435ab..04785b728b90b 100644 --- a/tests/components/imap/test_init.py +++ b/tests/components/imap/test_init.py @@ -30,12 +30,21 @@ from tests.common import MockConfigEntry, async_capture_events, async_fire_time_changed +@pytest.mark.parametrize( + "cipher_list", [None, "python_default", "modern", "intermediate"] +) @pytest.mark.parametrize("imap_has_capability", [True, False], ids=["push", "poll"]) async def test_entry_startup_and_unload( - hass: HomeAssistant, mock_imap_protocol: MagicMock + hass: HomeAssistant, mock_imap_protocol: MagicMock, cipher_list: str ) -> None: - """Test imap entry startup and unload with push and polling coordinator.""" - config_entry = MockConfigEntry(domain=DOMAIN, data=MOCK_CONFIG) + """Test imap entry startup and unload with push and polling coordinator and alternate ciphers.""" + config = MOCK_CONFIG.copy() + if cipher_list: + config["ssl_cipher_list"] = cipher_list + else: + config.pop("ssl_cipher_list") + + config_entry = MockConfigEntry(domain=DOMAIN, data=config) config_entry.add_to_hass(hass) assert await hass.config_entries.async_setup(config_entry.entry_id) await hass.async_block_till_done() From bc316931ac8aeb7ddbb4ef74644ef547bd4df5a3 Mon Sep 17 00:00:00 2001 From: jbouwh Date: Sun, 23 Apr 2023 13:12:38 +0000 Subject: [PATCH 2/5] Use client_context to get the ssl_context --- homeassistant/components/imap/coordinator.py | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/homeassistant/components/imap/coordinator.py b/homeassistant/components/imap/coordinator.py index 9df6c77631163..bdf1c2e67e121 100644 --- a/homeassistant/components/imap/coordinator.py +++ b/homeassistant/components/imap/coordinator.py @@ -6,7 +6,6 @@ from datetime import datetime, timedelta import email import logging -import ssl from typing import Any from aioimaplib import AUTH, IMAP4_SSL, NONAUTH, SELECTED, AioImapException @@ -22,7 +21,7 @@ from homeassistant.core import HomeAssistant from homeassistant.exceptions import ConfigEntryAuthFailed, ConfigEntryError from homeassistant.helpers.update_coordinator import DataUpdateCoordinator, UpdateFailed -from homeassistant.util.ssl import SSL_CIPHER_LISTS, SSLCipherList +from homeassistant.util.ssl import SSLCipherList, client_context from .const import ( CONF_CHARSET, @@ -43,14 +42,13 @@ async def connect_to_server(data: Mapping[str, Any]) -> IMAP4_SSL: """Connect to imap server and return client.""" - if ( - ciphers := data.get(CONF_SSL_CIPHER_LIST, SSLCipherList.PYTHON_DEFAULT) - ) != SSLCipherList.PYTHON_DEFAULT: - context = ssl.create_default_context() - context.set_ciphers(SSL_CIPHER_LISTS[SSLCipherList(ciphers)]) - client = IMAP4_SSL(data[CONF_SERVER], data[CONF_PORT], ssl_context=context) - else: - client = IMAP4_SSL(data[CONF_SERVER], data[CONF_PORT]) + client = IMAP4_SSL( + data[CONF_SERVER], + data[CONF_PORT], + ssl_context=client_context( + ssl_cipher_list=data.get(CONF_SSL_CIPHER_LIST, SSLCipherList.PYTHON_DEFAULT) + ), + ) await client.wait_hello_from_server() From f4a51a8e046dc970d2beb5b5d7783fceff8b790a Mon Sep 17 00:00:00 2001 From: jbouwh Date: Sun, 23 Apr 2023 13:14:41 +0000 Subject: [PATCH 3/5] Formatting --- homeassistant/components/imap/coordinator.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/homeassistant/components/imap/coordinator.py b/homeassistant/components/imap/coordinator.py index bdf1c2e67e121..07b55dc4788d6 100644 --- a/homeassistant/components/imap/coordinator.py +++ b/homeassistant/components/imap/coordinator.py @@ -42,13 +42,10 @@ async def connect_to_server(data: Mapping[str, Any]) -> IMAP4_SSL: """Connect to imap server and return client.""" - client = IMAP4_SSL( - data[CONF_SERVER], - data[CONF_PORT], - ssl_context=client_context( - ssl_cipher_list=data.get(CONF_SSL_CIPHER_LIST, SSLCipherList.PYTHON_DEFAULT) - ), + ssl_context = client_context( + ssl_cipher_list=data.get(CONF_SSL_CIPHER_LIST, SSLCipherList.PYTHON_DEFAULT) ) + client = IMAP4_SSL(data[CONF_SERVER], data[CONF_PORT], ssl_context=ssl_context) await client.wait_hello_from_server() From 2b99e3884c175070e46161111ef4c1f23c4a15c8 Mon Sep 17 00:00:00 2001 From: jbouwh Date: Sun, 23 Apr 2023 15:03:47 +0000 Subject: [PATCH 4/5] Add ssl error no make error handling more specific --- homeassistant/components/imap/config_flow.py | 6 ++++++ homeassistant/components/imap/strings.json | 3 ++- tests/components/imap/test_config_flow.py | 15 +++++++++++---- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/homeassistant/components/imap/config_flow.py b/homeassistant/components/imap/config_flow.py index 7bd34b71aff0a..605ebd21b5c10 100644 --- a/homeassistant/components/imap/config_flow.py +++ b/homeassistant/components/imap/config_flow.py @@ -3,6 +3,7 @@ import asyncio from collections.abc import Mapping +import ssl from typing import Any from aioimaplib import AioImapException @@ -78,6 +79,11 @@ async def validate_input(user_input: dict[str, Any]) -> dict[str, str]: errors[CONF_USERNAME] = errors[CONF_PASSWORD] = "invalid_auth" except InvalidFolder: errors[CONF_FOLDER] = "invalid_folder" + except ssl.SSLError: + # The aioimaplib library 1.0.1 does not raise an ssl.SSLError correctly, but is logged + # See https://github.com/bamthomas/aioimaplib/issues/91 + # This handler is added to be able to supply a better error message + errors["base"] = "ssl_error" except (asyncio.TimeoutError, AioImapException, ConnectionRefusedError): errors["base"] = "cannot_connect" else: diff --git a/homeassistant/components/imap/strings.json b/homeassistant/components/imap/strings.json index bcd7994f2df01..e50370dd9b1ca 100644 --- a/homeassistant/components/imap/strings.json +++ b/homeassistant/components/imap/strings.json @@ -26,7 +26,8 @@ "invalid_auth": "[%key:common::config_flow::error::invalid_auth%]", "invalid_charset": "The specified charset is not supported", "invalid_folder": "The selected folder is invalid", - "invalid_search": "The selected search is invalid" + "invalid_search": "The selected search is invalid", + "ssl_error": "An SSL error occurred. Change SSL cipher list and try again" }, "abort": { "already_configured": "[%key:common::config_flow::abort::already_configured_device%]", diff --git a/tests/components/imap/test_config_flow.py b/tests/components/imap/test_config_flow.py index 43275818c15e9..387173a397889 100644 --- a/tests/components/imap/test_config_flow.py +++ b/tests/components/imap/test_config_flow.py @@ -1,5 +1,6 @@ """Test the imap config flow.""" import asyncio +import ssl from unittest.mock import AsyncMock, patch from aioimaplib import AioImapException @@ -114,10 +115,16 @@ async def test_form_invalid_auth(hass: HomeAssistant) -> None: @pytest.mark.parametrize( - "exc", - [asyncio.TimeoutError, AioImapException("")], + ("exc", "error"), + [ + (asyncio.TimeoutError, "cannot_connect"), + (AioImapException(""), "cannot_connect"), + (ssl.SSLError, "ssl_error"), + ], ) -async def test_form_cannot_connect(hass: HomeAssistant, exc: Exception) -> None: +async def test_form_cannot_connect( + hass: HomeAssistant, exc: Exception, error: str +) -> None: """Test we handle cannot connect error.""" result = await hass.config_entries.flow.async_init( DOMAIN, context={"source": config_entries.SOURCE_USER} @@ -132,7 +139,7 @@ async def test_form_cannot_connect(hass: HomeAssistant, exc: Exception) -> None: ) assert result2["type"] == FlowResultType.FORM - assert result2["errors"] == {"base": "cannot_connect"} + assert result2["errors"] == {"base": error} # make sure we do not lose the user input if somethings gets wrong assert { From 2eb34e6a99fc47e413e27cabf5a8b5e6f70c13ab Mon Sep 17 00:00:00 2001 From: jbouwh Date: Mon, 24 Apr 2023 10:45:23 +0000 Subject: [PATCH 5/5] Make ssl_ciper_list an advanced option --- homeassistant/components/imap/config_flow.py | 15 +++++++++++---- tests/components/imap/test_config_flow.py | 5 ++--- tests/components/imap/test_init.py | 2 -- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/homeassistant/components/imap/config_flow.py b/homeassistant/components/imap/config_flow.py index 605ebd21b5c10..71b09048e6f1f 100644 --- a/homeassistant/components/imap/config_flow.py +++ b/homeassistant/components/imap/config_flow.py @@ -50,11 +50,13 @@ vol.Optional(CONF_CHARSET, default="utf-8"): str, vol.Optional(CONF_FOLDER, default="INBOX"): str, vol.Optional(CONF_SEARCH, default="UnSeen UnDeleted"): str, - vol.Optional( - CONF_SSL_CIPHER_LIST, default=SSLCipherList.PYTHON_DEFAULT - ): CIPHER_SELECTOR, } ) +CONFIG_SCHEMA_ADVANCED = { + vol.Optional( + CONF_SSL_CIPHER_LIST, default=SSLCipherList.PYTHON_DEFAULT + ): CIPHER_SELECTOR +} OPTIONS_SCHEMA = vol.Schema( { @@ -127,8 +129,13 @@ async def async_step_user( self, user_input: dict[str, Any] | None = None ) -> FlowResult: """Handle the initial step.""" + + schema = CONFIG_SCHEMA + if self.show_advanced_options: + schema = schema.extend(CONFIG_SCHEMA_ADVANCED) + if user_input is None: - return self.async_show_form(step_id="user", data_schema=CONFIG_SCHEMA) + return self.async_show_form(step_id="user", data_schema=schema) self._async_abort_entries_match( { diff --git a/tests/components/imap/test_config_flow.py b/tests/components/imap/test_config_flow.py index 387173a397889..82430549f05d9 100644 --- a/tests/components/imap/test_config_flow.py +++ b/tests/components/imap/test_config_flow.py @@ -28,7 +28,6 @@ "charset": "utf-8", "folder": "INBOX", "search": "UnSeen UnDeleted", - "ssl_cipher_list": "python_default", } MOCK_OPTIONS = { @@ -434,7 +433,6 @@ async def test_import_flow_success(hass: HomeAssistant) -> None: "charset": "utf-8", "folder": "INBOX", "search": "UnSeen UnDeleted", - "ssl_cipher_list": "python_default", } assert len(mock_setup_entry.mock_calls) == 1 @@ -474,7 +472,8 @@ async def test_config_flow_with_cipherlist( config = MOCK_CONFIG.copy() config["ssl_cipher_list"] = cipher_list result = await hass.config_entries.flow.async_init( - DOMAIN, context={"source": config_entries.SOURCE_USER} + DOMAIN, + context={"source": config_entries.SOURCE_USER, "show_advanced_options": True}, ) assert result["type"] == FlowResultType.FORM assert result["errors"] is None diff --git a/tests/components/imap/test_init.py b/tests/components/imap/test_init.py index 04785b728b90b..58bb084296abf 100644 --- a/tests/components/imap/test_init.py +++ b/tests/components/imap/test_init.py @@ -41,8 +41,6 @@ async def test_entry_startup_and_unload( config = MOCK_CONFIG.copy() if cipher_list: config["ssl_cipher_list"] = cipher_list - else: - config.pop("ssl_cipher_list") config_entry = MockConfigEntry(domain=DOMAIN, data=config) config_entry.add_to_hass(hass)