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

feat(BA-524): Deprecate X-BackendAI-SSO header for pipeline service authentication #3353

Merged

Conversation

rapsealk
Copy link
Member

@rapsealk rapsealk commented Jan 2, 2025

Follow-up of #503 and #1350. (BA-524)

This pull request deprecates the JWT-based X-BackendAI-SSO header to reduce complexity in authentication process for the pipeline service.

AS-IS

sequenceDiagram
    Client->>+Backend.AI Gateway: POST /pipeline/login/ (X-BackendAI-SessionID: ...)
    Backend.AI Gateway-->>+Backend.AI FastTrack: POST /login/ (X-BackendAI-SessionID: ..., X-BackendAI-SSO: ...)
    Backend.AI FastTrack-->>-Backend.AI Gateway: 200 OK (X-BackendAI-SSO: ...)
    Backend.AI Gateway-->>-Client: 200 OK (X-BackendAI-SSO: ...)
    Client->>+Backend.AI Gateway: POST /pipeline/.../ (X-BackendAI-SessionID: ..., X-BackendAI-SSO: ...)
    Backend.AI Gateway-->>+Backend.AI FastTrack: POST /.../ (X-BackendAI-SessionID: ..., X-BackendAI-SSO: ...)
    Backend.AI FastTrack-->>-Backend.AI Gateway: 200 OK
    Backend.AI Gateway->>-Client: 200 OK
Loading

TO-BE

sequenceDiagram
    Client->>+Backend.AI Gateway: POST /pipeline/login/ (X-BackendAI-SessionID: ...)
    Backend.AI Gateway-->>+Backend.AI FastTrack: POST /login/ (X-BackendAI-SessionID: ...)
    Backend.AI FastTrack-->>-Backend.AI Gateway: 200 OK {"token": "AUTH_TOKEN"}
    Backend.AI Gateway-->>-Client: 200 OK {"token": "AUTH_TOKEN"}
    Client->>+Backend.AI Gateway: POST /pipeline/.../ (Authorization: "Bearer AUTH_TOKEN")
    Backend.AI Gateway-->>+Backend.AI FastTrack: POST /.../ (Authorization: "Bearer AUTH_TOKEN")
    Backend.AI FastTrack-->>-Backend.AI Gateway: 200 OK
    Backend.AI Gateway->>-Client: 200 OK
Loading

Checklist: (if applicable)

  • Milestone metadata specifying the target backport version
  • Mention to the original issue

@rapsealk rapsealk added this to the 24.12 milestone Jan 2, 2025
@github-actions github-actions bot added comp:webserver Related to Web Server component size:M 30~100 LoC labels Jan 2, 2025
Comment on lines 211 to 214
if proxy_path == "pipeline" and real_path.rstrip("/") == "login":
api_rqst.headers["X-BackendAI-SessionID"] = request.headers.get(
"X-BackendAI-SessionID", ""
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont' think rqst is a commonly used abbreviation. Would it be possible to rename it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renaming the api_rqst will be a over-scoped task, as the name is used in various modules in this project.
Would you mind if I issue a new ticket for it and proceed in next work?
Thank you.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please create the rename task with priority: low and handle it when you have time. :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task will be tracked by BA-469

Comment on lines -210 to -226
if proxy_path == "pipeline":
session_id = request.headers.get("X-BackendAI-SessionID", "")
if not (sso_token := request.headers.get("X-BackendAI-SSO")):
jwt_secret = config["pipeline"]["jwt"]["secret"]
now = datetime.now().astimezone()
payload = {
# Registered claims
"exp": now + timedelta(seconds=config["session"]["max_age"]),
"iss": "Backend.AI Webserver",
"iat": now,
# Private claims
"aiohttp_session": session_id,
"access_key": api_session.config.access_key, # since 23.03.10
}
sso_token = jwt.encode(payload, key=jwt_secret, algorithm="HS256")
api_rqst.headers["X-BackendAI-SSO"] = sso_token
api_rqst.headers["X-BackendAI-SessionID"] = session_id
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does Fasttrack handle version compatibility?
When making changes this time, is there any risk of breaking functionality for clients using previous versions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is recommended to use the same version as Backend.AI Core.
To enhance version compatibility, we may adopt a dedicated header (e.g., X-BackendAI-FastTrack-Version).

Comment on lines 165 to 168
if real_path.rstrip("/") == "login":
api_session = await asyncio.shield(get_api_session(request, endpoint))
else:
api_session = await asyncio.shield(get_anonymous_session(request, endpoint))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there another way to identify it as a login path? Relying on the real_path could be fragile.

@rapsealk rapsealk marked this pull request as draft January 15, 2025 05:32
@rapsealk rapsealk marked this pull request as ready for review January 15, 2025 05:52
@rapsealk rapsealk requested a review from HyeockJinKim January 15, 2025 05:52
*,
is_anonymous: bool = False,
api_endpoint: Optional[str] = None,
extra_forwarding_headers: Iterable[str] | None = None,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any suggestion?

Suggested change
extra_forwarding_headers: Iterable[str] | None = None,
extra_http_headers_to_forward: Iterable[str] | None = None,

Comment on lines 203 to 205
for hdr in {*HTTP_HEADERS_TO_FORWARD, *extra_forwarding_headers}:
if request.headers.get(hdr) is not None:
api_rqst.headers[hdr] = request.headers[hdr]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The header seems to allow being overridden, which poses a potential security risk.

Copy link
Member Author

@rapsealk rapsealk Jan 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolves by 143cc14 + 91f7220.

Comment on lines 657 to 667
pipeline_api_endpoint = config["pipeline"]["endpoint"]
pipeline_handler = partial(web_handler, is_anonymous=True, api_endpoint=pipeline_api_endpoint)
pipeline_login_handler = partial(
web_handler,
is_anonymous=False,
api_endpoint=pipeline_api_endpoint,
extra_forwarding_headers={"X-BackendAI-SessionID"},
)
pipeline_websocket_handler = partial(
websocket_handler, is_anonymous=True, api_endpoint=pipeline_api_endpoint.with_scheme("ws")
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be cleaner to separate and pass it from the handler layer. Please create a follow-up issue for refactoring.

@github-actions github-actions bot added the size:L 100~500 LoC label Jan 16, 2025
@github-actions github-actions bot removed the size:M 30~100 LoC label Jan 16, 2025
@rapsealk rapsealk requested a review from HyeockJinKim January 16, 2025 08:26
@rapsealk rapsealk modified the milestones: 24.12, 25Q1 Jan 16, 2025
Copy link
Collaborator

@HyeockJinKim HyeockJinKim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@rapsealk rapsealk changed the title feat(EN-115): Deprecate X-BackendAI-SSO header for pipeline service authentication feat(BA-524): Deprecate X-BackendAI-SSO header for pipeline service authentication Jan 16, 2025
@HyeockJinKim HyeockJinKim added this pull request to the merge queue Jan 17, 2025
Merged via the queue into main with commit 3ab69eb Jan 17, 2025
22 checks passed
@HyeockJinKim HyeockJinKim deleted the feature/EN-115-deprecate-pipeline-authentication-jwt branch January 17, 2025 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:webserver Related to Web Server component size:L 100~500 LoC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants